elaird / supy

analyze events stored in TTrees in parallel
8 stars 7 forks source link

Pep8 #174

Closed elaird closed 10 years ago

elaird commented 11 years ago

What would you guys think of following this guide [1]? I used this program [2] to check the three files included in the pull request.

Ted

[1] http://www.python.org/dev/peps/pep-0008/ [2] https://github.com/jcrocholl/pep8

gerbaudo commented 11 years ago

Hi Ted and Burt,

I was trying myself to read the pep8 guide a couple of weeks ago, and I like the idea of using a checker to verify that my code follows the guidelines (it's a good way for me to get into the habit of using the right idiom...). However, there are two 'features' that I liked in the current 'supy style':

  1. space before :, because it makes it easier to see
  2. in-lined statement for if and else when they involve only a single statement, because it makes the code more compact and easier to follow

For 1), we could add ignore = E203 in ~/.config/pep8, but I don't see a similar option for 2). Anyway, these are only my personal preferences, and I don't have any strong objection against using pep8.

Davide

betchart commented 11 years ago

Hi Ted,

My thoughts on PEP8 are multiple and conflicting.

  1. In general, I like adhering to standards, and in particular, this standard seems to have the right motivation, which is to make the code more readable. We've come a long way from C++ already on the readability front, and I appreciate that it is of great importance. If we are going to adopt an explicit set of rules, these seem appropriate.
  2. There exist coding styles that I think are terrible, but are adhered to without straying. For example, ROOT classes and member names, T* and f, Roo.
  3. I like the note at the beginning of PEP8 that makes it clear this standard should not be followed when it makes the code less clear to read, even for one used to reading PEP8 formatted code.
  4. I think we are already adhering to a large number of these conventions, enforced or made easy by the use of EMACS and git:
    • tabs/spaces
    • alignment
    • superfluous whitespace.
    • encodings?
    • className style (except with calculables that get prefixes)
    • __private members
  5. Some of the conventions we violate often or with consistency are:
    • spacing around = for named arguments
    • multiline import statements
    • maximum line length
    • function names in lower_case
  6. Personal RAM --- I suspect that the part of PEP8 most difficult for me to accept would be the maximum line length enforcement. As you probably know from experience, I have a preference for high vertical information density. Github shows that while other supy contributors have roughtly twice as many additions as deletions, my statistics are just the opposite, probably due in part to my tendency to condense. I find that I can understand what is going on much faster if I do not have to change the screen up or down to keep reading the code. Switching the screen is kind of like swapping out to disk for me, and it gets hard to compare interactions. I worry that adhering to PEP8 strictly will cause the number of lines of code to roughly double (see __master__.py before and after), resulting in half the vertical information density, therefore slowing my reading. On the other hand, I find it annoying when the code exceeds the horizontal length displayed by github (125 characters default chromium for me, but non-constant with changing text size). I'd be quite a bit more willing to try to keep the code within 125 lines than within 80, and hope you'll support that.
  7. We have quite a number of implicit conventions already in supy, for example, with calculables and steps and utilities. Before changing any widely used supy convention en masse to adhere to PEP8, I would prefer to discuss it, on a case by case basis.

Burt

betchart commented 11 years ago

Would like to express reasonable level of support for Davide's (1) and (2). Burt

betchart commented 11 years ago

Hi Ted,

I have played more with a PEP8 checker, and I am supportive of a goal to be PEP8 compatible with some exceptions. I would prefer to --ignore errors

Davide mentioned his preference to also --ignore

Regarding E501, I would support allowing the line length to be maximum 125 characters, rather than 80 characters.

Please see branch not-pep8 for a version of __master__.py conforming to PEP8 --ignore E501,E701

Burt