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

TravisCI: Check for PEP8 #298

Merged
merged 9 commits into from May 8, 2016
Merged

TravisCI: Check for PEP8 #298

merged 9 commits into from May 8, 2016

Conversation

@tammoippen
Copy link
Contributor

@tammoippen tammoippen commented Apr 7, 2016

This PR lets TravisCI check changed Python files for PEP8 compliance, as requested by #188. The file extras/check_code_style.sh now also checks for PEP8.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 11, 2016

👍 from my side. @apeyser, would you take a look, too?

@@ -111,6 +111,9 @@ else
fi
format_error_files=""

# Ignore those PEP8 rules
PEP8_IGNORES="E121,E123,E126,E226,E24,E704"

This comment has been minimized.

@apeyser

apeyser Apr 11, 2016
Contributor

Should the E24 be E24x? I don't see an E24 in the pep8 script's documentation.
Where did this particular choice of pep8 exceptions come from?

This comment has been minimized.

@tammoippen

tammoippen Apr 11, 2016
Author Contributor

Hi @apeyser,

Without it TravisCI complains, and my local machine not: extras/parse_travis_log.py:285:45: E126 continuation line over-indented for hanging indent
The line looks much nicer with this over-indention. On my Mac when I execute pep8 --help I get:

pep8 --help
Usage: pep8 [options] input ...

Options:
...
  --ignore=errors      skip errors and warnings (e.g. E4,W) (default:
                       E121,E123,E126,E226,E24,E704)
  --show-source        show source code for each error

i.e. E126 is ignored here by default. Maybe we should only ignore E126? Also see here for discussion.

This comment has been minimized.

@apeyser

apeyser Apr 11, 2016
Contributor

My python3?-pep8 doesn't give me the defaults. Maybe I should pull up the script and see what the defaults actually are.

build.sh Outdated
*.py )
set +e # pep8 returns 1 if the file does not comply with PEP8
echo "Check PEP8 on file $f:"
pep8_result=`pep8 --first $f`

This comment has been minimized.

@apeyser

apeyser Apr 11, 2016
Contributor

Why not just

if ! pep8_result=`pep8 --first $f`; then
filename = line.split(' ')[-1].strip()[0:-1]
d = set()
res = {filename: d}
while True:

This comment has been minimized.

@apeyser

apeyser Apr 11, 2016
Contributor

Options here:

fiter = iter(f)
for line in fiter:

and use the same iter at 206
or ... comment what's going on here with walking through the file.

This comment has been minimized.

@tammoippen

tammoippen Apr 20, 2016
Author Contributor

I go through the file only once and f stores the "pointer" into where we are at reading the file. When i use iter(f), each function has its own state there, correct? Then I have to rewrite the script, which might be not a bad idea, as it will make the individual parsing functions simpler. But I think this would be a topic for another PR.

This comment has been minimized.

@tammoippen

tammoippen Apr 20, 2016
Author Contributor

I go through the file only once and f stores the "pointer" into where we are at reading the file. When i use iter(f), each function has its own state there, correct? Then I have to rewrite the script, which might be not a bad idea, as it will make the individual parsing functions simpler. But I think this would be a topic for another PR.

This comment has been minimized.

@apeyser

apeyser Apr 25, 2016
Contributor

In fact, no -- the underlying file handles moves forward, so:

f = open(file, 'r')
f1 = iter(f)
f2 = iter(f)
for l in f1: print l

will move f2 forward as well as f1. But in general for iterators, you would be right. And you're probably right that everything should just move the iterator instead of the file handle around... but it is too wide for this pr.

I'd just do:

for line in f: ....

since that's anyway the same thing.

This comment has been minimized.

@tammoippen

tammoippen Apr 25, 2016
Author Contributor

There is another nested loop continuing the iteration throught the file:

if line.startswith('+echo ' + filename):
    while True:
        line = f.readline()

Does it work there, too?

This comment has been minimized.

@apeyser

apeyser Apr 27, 2016
Contributor

Should, since the iterator is simply an adaptor on the common underlying buffer and file descriptor at the very bottom.

This comment has been minimized.

@tammoippen

tammoippen Apr 27, 2016
Author Contributor

@apeyser I structured the script with the readline() method. When I replace it with the iterator, I get errors like:

ValueError: Mixing iteration and read methods would lose data

As this is not directly related to PEP8, I would like to put this into another issue, and here just document, what the lines are intended to do.

This comment has been minimized.

@apeyser

apeyser Apr 27, 2016
Contributor

@tammoippen

Sounds reasonable. I see in the documentation that the read-ahead buffer is broken, that it's only used in next calls but not readline calls. It's even built on top of a FILE*, so that you have buffers of buffers on top of the POSIX buffers on top of the hardware buffers...
(This is also the answer to why I'm suspicious of all directives coming out of that design camp)

echo "Check PEP8 on file $f:"
pep8_result=`pep8 --first --ignore=$PEP8_IGNORES $f`

if [ "$?" -gt "0" ]; then

This comment has been minimized.

@apeyser

apeyser Apr 12, 2016
Contributor

Why not: if ! pep8_result=pep8 ... ?

echo "Perform '$CLANG_FORMAT -i <filename>' on them, otherwise TravisCI will report failure!"
echo "TravisCI will report failure!"
echo "- On C/CPP files, perform '$CLANG_FORMAT -i <filename>' on them."
echo "- On Python files, perform 'pep8ify -w <filename>' on them and"

This comment has been minimized.

@jougs

jougs Apr 27, 2016
Contributor

Please remove the suffix "on them". See here.

equal sign is a space separated list of changed files in the PR. Returns that
list.
Run through the file f until a line starts with '+file_names='.
After the equal sign is a space separated list of changed files in the PR.

This comment has been minimized.

@jougs

jougs Apr 27, 2016
Contributor

The equal sign is followed by a space separated list of changed files in the PR.

tammoippen added 9 commits Jan 18, 2016
# Conflicts:
#	extras/parse_travis_log.py
# Conflicts:
#	extras/parse_travis_log.py
E126 "continuation line over-indented for hanging indent"

Is ignored in later pep8 versions
@tammoippen tammoippen force-pushed the tammoippen:pep8_issue188 branch from c5babad to 11f40fb Apr 28, 2016
@apeyser
Copy link
Contributor

@apeyser apeyser commented May 8, 2016

👍

@apeyser apeyser merged commit 7df53d7 into nest:master May 8, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tammoippen tammoippen deleted the tammoippen:pep8_issue188 branch May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.