nexusformat / features

Work on NeXus features and recipes by the NIAC and friends
0 stars 6 forks source link

Make code conform to PEP 8 #5

Closed matthew-d-jones closed 7 years ago

matthew-d-jones commented 7 years ago

These changes aim to improve readability and consistency by making the code conform to PEP 8, with the exception of E501 (79 character line length restriction) which is usually considered deprecated.

The code now passes flake8 with flake8 --ignore=E501.

zjttoefs commented 7 years ago

Looks like I'm not allowed to write Python code, I struggle to conform to the style guide. :-(

But I think it makes sense to enforce that on code that is meant to be an example.

We have this list of restrictions on the code here:

That currently lives on http://idregistry-nexusformat.rhcloud.com/ I should move the repository for that here, but there would be a point adding that to the README here as well. With the addition of PEP8.

matthew-d-jones commented 7 years ago

@zjttoefs Thanks for merging :) Using a nice IDE which will tell you when you are not conforming to PEP 8 helps (my favourite: https://www.jetbrains.com/pycharm/)

It may not be a good idea to put any barrier for people to add features, but enforcing PEP 8 on example code and nxfeature.py might be good? I could add a Flake8 check for that to the Travis config and a note on the README that PEP 8 is encouraged?

zjttoefs commented 7 years ago

I think we can "require" PEP 8 compliance, but not have it enforced by a tool, for contributed features. At least not as the only thing we test for. If we could also test the rules on imports and maybe check for obvious I/O violations, maybe being strict on the code could be seen as sensible. The recipe is to a high degree meant for human consumption. Readability is important. Not sure I would go for the character limit per line, though.

Most of my commits are straight out of "vi" - without syntax highlighting or any other cool features. Running pycharm over my modem connection might not work so well.^^ CARRIER LOST