Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEP8 style change #84

Closed
elfosardo opened this issue Apr 18, 2019 · 6 comments
Closed

PEP8 style change #84

elfosardo opened this issue Apr 18, 2019 · 6 comments

Comments

@elfosardo
Copy link
Collaborator

Currently the allowed line length for tox tests is 160 characters, which is a bit too much according to standard PEP8 convention.
We should remove the max-line-length option from tox.ini and change the code accordingly.
It's always possible to have special cases and exclude checks to favour readability, for example using # noqa

@tbreeds
Copy link
Collaborator

tbreeds commented Jul 19, 2019

I'm okay with doing this but I'd like us to have a very good look at which other PEP8 style things we want to do and just do series of conversions that are only pep8 related and the most obvious changes only.

It will of course force and and all open PR to conflict so we need to be mindful of that

@elfosardo
Copy link
Collaborator Author

@tbreeds thanks for your answer, what I had in mind with this was just a start towards better defined pep8 rules.
When opening a PR, I usually try to stick to the 80 characters rule and change the module I'm working on, but I agree we should just have pep8 related changes to not mix with code change.
Ideally, after defining a set of style rules we'd like to have I'd go module by module one at a time and apply them.
My starting proposal for the pep8 rules:

  • Remove the 160 characters limit and have a standard 80 characters instead.
  • Keep ignoring E123 and E125, but remove H803 and H302 from the ignore list.
  • Add to enable-extensions:
    [H106] Don't put vim configuration in source files.
    [H203] Use assertIs(Not)None to check for None.
    [H204] Use assert(Not)Equal to check for equality.
    [H205] Use assert(Greater|Less)(Equal) for comparison.

It's a lot of change to have all at the same time, so we could also apply only 1 or 2 rules at a time.

@elfosardo elfosardo changed the title Limit line length to 80 characters PEP8 style change Jul 19, 2019
@tbreeds
Copy link
Collaborator

tbreeds commented Jul 19, 2019

My starting proposal for the pep8 rules:

  • Remove the 160 characters limit and have a standard 80 characters instead.
  • Keep ignoring E123 and E125, but remove H803 and H302 from the ignore list.

I'd actually (I think) like to see E123 and E125 removed from the ignore list
H803 is trivial to get rid of. I think removing H302 would depend on what the pacth looked like. Personally I'm okay with importing parts of modules esp when the code is clean but I could easily be convinced.

  • Add to enable-extensions:
    [H106] Don't put vim configuration in source files.
    [H203] Use assertIs(Not)None to check for None.
    [H204] Use assert(Not)Equal to check for equality.
    [H205] Use assert(Greater|Less)(Equal) for comparison.

Sure they all look fine to me.

It's a lot of change to have all at the same time, so we could also apply only 1 or 2 rules at a time.

Yup. I guess step one is upgrade to a 'new' version of hacking, step two is the line length an then it's just one rule at a time. It's a lot of churn but if you have time to do it I'll make time to review it

@elfosardo
Copy link
Collaborator Author

I did a quick test and removing E123,E125,H803 didn't actually change anything, although I think E123 and E125 are actually invalid pep8, so I still suggest to leave them.

We can also leave H302 for the time being.

There is also some refactoring to do on the docstrings in all the modules, but we'll get there.

@elfosardo
Copy link
Collaborator Author

First simple patch #99

elfosardo added a commit to elfosardo/hardware that referenced this issue Aug 9, 2019
As discussed in redhat-cip#84 we should enforce line length limit of 80
columns everywhere.
All the changes in the code were already submitted in other
patches, this patch enforces the change in the tests.
elfosardo added a commit to elfosardo/hardware that referenced this issue Aug 9, 2019
As discussed in redhat-cip#84 we should enforce line length limit of 80
columns everywhere.
Some changes are needed in various modules to be able to apply
the change, all changes are cosmetic and don't modify logic or
features.
@elfosardo
Copy link
Collaborator Author

Considering the latest changes, I think this can be closed.
Feel free to re-open it if you feel there's more to discuss.

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

No branches or pull requests

2 participants