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 compliance for existing codebase #1375

Closed
EliteMasterEric opened this issue Apr 23, 2018 · 2 comments
Closed

PEP8 compliance for existing codebase #1375

EliteMasterEric opened this issue Apr 23, 2018 · 2 comments

Comments

@EliteMasterEric
Copy link
Contributor

EliteMasterEric commented Apr 23, 2018

I am currently working on a pull request for this, using Python's PEP8 tool, but in the meantime, I'm posting an issue so we can discuss the changes involved (such as whether certain issues could/should be ignored).

Running pep8 --statistics ./src/ results in the following:

29      E101 indentation contains mixed spaces and tabs
148     E111 indentation is not a multiple of four
7       E114 indentation is not a multiple of four (comment)
4       E115 expected an indented block (comment)
4       E116 unexpected indentation (comment)
3       E124 closing bracket does not match visual indentation
4       E125 continuation line with same indent as next logical line
46      E127 continuation line over-indented for visual indent
47      E128 continuation line under-indented for visual indent
99      E201 whitespace after '('
66      E202 whitespace before ']'
63      E203 whitespace before ':'
18      E211 whitespace before '('
187     E221 multiple spaces before operator
6       E222 multiple spaces after operator
124     E225 missing whitespace around operator
6       E227 missing whitespace around bitwise or shift operator
1290    E228 missing whitespace around modulo operator
407     E231 missing whitespace after ','
110     E251 unexpected spaces around keyword / parameter equals
366     E261 at least two spaces before inline comment
244     E262 inline comment should start with '# '
500     E265 block comment should start with '# '
335     E266 too many leading '#' for block comment
3       E271 multiple spaces after keyword
6       E272 multiple spaces before keyword
131     E301 expected 1 blank line, found 0
1076    E302 expected 2 blank lines, found 1
113     E303 too many blank lines (2)
18      E401 multiple imports on one line
268     E402 module level import not at top of file
3938    E501 line too long (104 > 79 characters)
52      E502 the backslash is redundant between brackets
208     E701 multiple statements on one line (colon)
9       E702 multiple statements on one line (semicolon)
10      E703 statement ends with a semicolon
41      E711 comparison to None should be 'if cond is not None:'
14      E712 comparison to True should be 'if cond is True:' or 'if cond:'
59      E713 test for membership should be 'not in'
3       E731 do not assign a lambda expression, use a def
26      W191 indentation contains tabs
1484    W291 trailing whitespace
2218    W293 blank line contains whitespace
83      W391 blank line at end of file
1       W503 line break before binary operator
37      W601 .has_key() is deprecated, use 'in'
1       W604 backticks are deprecated, use 'repr()'

Many issues (such as trailing whitespace and blank line count) can be easily and automatically resolved across all files using regular expressions or other automation/scripts.

I think that resolving all these issues now, and requiring as much compliance as possible for future changes, will resolve many future problems, such as the long delays imposed on my pull request, #1154.

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Apr 23, 2018

Some lines may use PEP8 violations for the purpose of readability.
For example, rosbag_main.py violates indentation rules to make code more logically organized and readable.

parser.add_option("-a", "--all", dest="all", default=False, action="store_true", help="record all topics")

Should we ignore issues like E221 or E501 for the sake of these lines? What about other parts of the same script, or in different scripts, where these rules are violated for no justifiable reason?

@mikepurvis
Copy link
Member

Historically, we haven't allowed style-only changes on the grounds that it makes backporting changes much more difficult to manage. However, there will be an opportunity in May 2019 to potentially make a style change as Lunar and Indigo will simultaneously be going out of support, so it would only be kinetic-devel and melodic-devel which would need to be simultaneously updated.

@EliteMasterEric EliteMasterEric closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2023
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