Add some linting jobs for CI #1807
Conversation
Currently this just runs ruff on python code, more checks to come.
dcbaker
force-pushed
the
submit/ci-lint
branch
from
e102fb2 to
0b2feba
Compare
| @@ -101,3 +102,4 @@ def main(): | |||
|
|
|||
| if __name__ == "__main__": | |||
| main() | |||
|
|
|||
There was a problem hiding this comment.
what do you know, a file with a missing new line. The very thing that started this :)
There was a problem hiding this comment.
Normally github's view of this would have indicated via some kind of red circle when the previous version didn't have a trailing end of line character, I thought. Meanwhile, the actual display here is this adds a blank line 105 to go with the code line 104. Files ending with main() are a problem and files ending with main()\n are correct... but if I checkout your PR, the file ends with main()\n\n, which is surely wrong?
In fact, if I checkout the file in git master locally, it doesn't appear to be missing the trailing end of line character.
| for l in x.splitlines(): | ||
| if b'examples' in l: | ||
| lines.append(str(l.replace(b'\\',b'/'))) | ||
| for line in x.splitlines(): | ||
| if b'examples' in line: | ||
| lines.append(str(line.replace(b'\\',b'/'))) |
There was a problem hiding this comment.
This specific ruff "issue" is extremely opinionated and also wrong.
There was a problem hiding this comment.
I get that it's opinionated, and in this case I'd agree waht was there before was fine, I think the second loop is more readaable with a line vs l though, since it's a long loop and things like line[len('MY_REALLY_LONG_STRING):] are generally more readable than l[len('MY_REALLY_LONG_STRING):]
| import os, sys | ||
| import os | ||
| import sys |
There was a problem hiding this comment.
Can we follow standard python coding guidelines? yes please
There was a problem hiding this comment.
Nothing standard about this change, other than that black encourages it.
There was a problem hiding this comment.
There was a problem hiding this comment.
The authors and maintainers of the cpython PEPs take every possible opportunity they possibly can to tell people that PEP 8 is not a standard for anything other than "the style for internal cpython project source code" and that people should stop putting them in a position to be the arbiters of python style, funnily enough.
And this particular rule is inconsistent and ugly so I can kind of see the point. :)
| @@ -121,7 +121,7 @@ def is_linux() -> bool: | |||
| return platform.system().lower() == 'linux' | |||
|
|
|||
| def is_windows() -> bool: | |||
| return platform.system().lower() == 'windows' and not "MSYSTEM" in os.environ | |||
| return platform.system().lower() == 'windows' and "MSYSTEM" not in os.environ | |||
There was a problem hiding this comment.
I can't tell is this is a bugfix.
|
Please rebase |
This is currently a draft since it only affects python files ATM. I'd like to also lint JSON and meson files.