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

Pep8ify all Python code in NEST #337

Merged
merged 8 commits into from May 11, 2016

Conversation

Projects
None yet
3 participants
@heplesser
Contributor

heplesser commented May 11, 2016

This PR makes all Python code in NEST PEP8 compliant. For code in some directories (all examples dirs and the scripts for the topology user manual), E402 (import not at beginning of file) is suppressed, because it is didactically useful to have imports in several places throughout the script.

@jougs @tammoippen @apeyser Could you review?

@@ -232,7 +239,13 @@ for f in $file_names; do
*.py )
echo "Check PEP8 on file $f:"
if ! pep8_result=`pep8 --first --ignore=$PEP8_IGNORES $f` ; then
if [[ $f =~ $EXAMPLE_DIRS ]]; then

This comment has been minimized.

@apeyser

apeyser May 11, 2016

Contributor

Isn't there a comment flag within the file to mark sections that should be ignored by pep8? I suggest it only to avoid a proliferation of cases here where ignores should be changed.

@apeyser

apeyser May 11, 2016

Contributor

Isn't there a comment flag within the file to mark sections that should be ignored by pep8? I suggest it only to avoid a proliferation of cases here where ignores should be changed.

This comment has been minimized.

@heplesser

heplesser May 11, 2016

Contributor

@apeyser As far as I have been able to find out, the only possibility is to append # noqa to pertaining lines. I have used that very sparingly. I have not been able to find a way to disable specific PEP8 errors for individual files or sections of files by comments in the files. In example code that will go onto our website or scripts that are included in the Topology manual, the # noqa comment seems undesirable because it would distract readers.

@heplesser

heplesser May 11, 2016

Contributor

@apeyser As far as I have been able to find out, the only possibility is to append # noqa to pertaining lines. I have used that very sparingly. I have not been able to find a way to disable specific PEP8 errors for individual files or sections of files by comments in the files. In example code that will go onto our website or scripts that are included in the Topology manual, the # noqa comment seems undesirable because it would distract readers.

This comment has been minimized.

@apeyser

apeyser May 11, 2016

Contributor

@heplesser I just checked it -- PyCQA/pycodestyle#27 (comment) ; Not possible, and apparently will never be possible -- it's a "philosophical" problem until someone forks pep8. The kind of philosophical problems that tend to crop up particularly often in the python community.

@apeyser

apeyser May 11, 2016

Contributor

@heplesser I just checked it -- PyCQA/pycodestyle#27 (comment) ; Not possible, and apparently will never be possible -- it's a "philosophical" problem until someone forks pep8. The kind of philosophical problems that tend to crop up particularly often in the python community.

This comment has been minimized.

@apeyser

apeyser May 11, 2016

Contributor

@heplesser I just found flake8 which is a wrapper around pep8 to make pep8 less religious, including turning off warnings on a file by file basis. Maybe worthwhile using?

@apeyser

apeyser May 11, 2016

Contributor

@heplesser I just found flake8 which is a wrapper around pep8 to make pep8 less religious, including turning off warnings on a file by file basis. Maybe worthwhile using?

This comment has been minimized.

@heplesser

heplesser May 11, 2016

Contributor

@apeyser Yes, that sounds interesting. Would you open a new issue for that?

@heplesser

heplesser May 11, 2016

Contributor

@apeyser Yes, that sounds interesting. Would you open a new issue for that?

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser May 11, 2016

Contributor

👍

Contributor

apeyser commented May 11, 2016

👍

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj May 11, 2016

Contributor

running pep8 from the command line for me still complains about three things:

./extras/parse_travis_log.py:327:45: E126 continuation line over-indented for hanging indent
./testsuite/manualtests/test_facetshw_stdp.py:137:25: E126 continuation line over-indented for hanging indent
./pynest/nest/tests/compatibility.py:37:30: E701 multiple statements on one line (colon)

Contributor

jakobj commented May 11, 2016

running pep8 from the command line for me still complains about three things:

./extras/parse_travis_log.py:327:45: E126 continuation line over-indented for hanging indent
./testsuite/manualtests/test_facetshw_stdp.py:137:25: E126 continuation line over-indented for hanging indent
./pynest/nest/tests/compatibility.py:37:30: E701 multiple statements on one line (colon)

Another attempt with some more fixes. One problem is that I have pep8…
… 1.7.0, while Travis uses 1.4.6.

Have also added selective exlusion of E402  to build.sh.
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 11, 2016

Contributor

@jakobj Could you recheck the issues pep8 flagged on your machine?

Contributor

heplesser commented May 11, 2016

@jakobj Could you recheck the issues pep8 flagged on your machine?

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj May 11, 2016

Contributor

@heplesser with the newest changes, pep8 does not complain about files anymore 👍

Contributor

jakobj commented May 11, 2016

@heplesser with the newest changes, pep8 does not complain about files anymore 👍

@heplesser heplesser merged commit 02844e7 into nest:master May 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@heplesser heplesser deleted the heplesser:pep8ify branch May 11, 2016

@steffengraber steffengraber referenced this pull request May 20, 2016

Merged

Help build #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment