Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

PEP8-conformity #137

Closed
JeppeKlitgaard opened this issue Feb 21, 2015 · 12 comments · Fixed by #276
Closed

PEP8-conformity #137

JeppeKlitgaard opened this issue Feb 21, 2015 · 12 comments · Fixed by #276

Comments

@JeppeKlitgaard
Copy link

Is there any reason why oauth2client is using two spaces for indentation instead of four, as recommended by PEP 8?

Would people be opposed to switching to PEP8-friendly code?

@anthmgoogle
Copy link

It might be related to keeping the lines to 80 characters, which would cause more wrapping with 4 characters. Certainly a disruptive change that should not be done lightly or not while there was a lot of work in progress.

@dhermes
Copy link
Contributor

dhermes commented Mar 24, 2015

There is a very simple reason, the Google internal style guide.

Though looking at the current style guide, maybe Google has moved back to using 4 spaces internally? (I left in 2013.)

@craigcitro
Copy link
Contributor

@dhermes is right -- this code was originally written to (mostly) follow the internal google python style guide. it's still 2 spaces.

i think everyone would be amenable to switching to full pep8, but as @anthmgoogle points out we'd cause a lot of churn for outstanding PRs. @nathanielmanistaatgoogle might be able to pick a good point to decree "we heretofore will use pep8".

@dhermes
Copy link
Contributor

dhermes commented Mar 25, 2015

We did this in gcloud-python. I used pep8ify to change the indents and made sure that git diff --ignore-all-space was empty.

After that I had to use a custom script to indent the docstrings.

From there, getting pylint compliance took careful tweaking of our config and submitting PRs by hand.

@nathanielmanistaatgoogle
Copy link
Contributor

I'm not opposed to it, but spell out the benefit of switching?

@craigcitro
Copy link
Contributor

external users don't see our code and think "why does this all look funny"?

@dhermes
Copy link
Contributor

dhermes commented Aug 14, 2015

@nathanielmanistaatgoogle WDYT about doing this?

@nathanielmanistaatgoogle
Copy link
Contributor

I think very highly of it happening. You've made most of the changes in the recent past and would be in the best position to judge the right time to do it. I'm personally sweet on pyformat and hope it is up to the task.

@dhermes
Copy link
Contributor

dhermes commented Aug 19, 2015

@nathanielmanistaatgoogle Looking at https://github.com/google/oauth2client/pulls I think now is as good a time as any to make this happen.

How do we want to usher it in?

Then I dream of moving on to #212 and then to #128.

@nathanielmanistaatgoogle
Copy link
Contributor

Run autopep8 and see how it looks? Run pyformat and see how it looks? Decide which one you like more? Decide which one you trust more? Have you any experience with either tool?

"Manually" is the last manner in which I would like to usher in such a change.

@dhermes
Copy link
Contributor

dhermes commented Aug 19, 2015

I've been down this road once before: googleapis/google-cloud-python#179

@dhermes
Copy link
Contributor

dhermes commented Aug 20, 2015

Here I go...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants