Add pylint to CI and fix all Python #3052
Conversation
|
Thanks for that! |
|
I'm not sure how much time I can spend in the next days on the review but I'll try to fit it in some time. In the meantime you could maybe look if the CONTRIBUTING.md file needs changes. The maintainers should also decide where to add your name - if you want to be displayed as contributor. |
I think so? But probably once this PR is approved and very close to a merge. We'd then regroup your commits/squash them and have the contributor addition as separate commit. |
There was a problem hiding this comment.
See build: https://github.com/ann0see/jamulus/actions/runs/4812734521 that's one of the tests.
|
Next step: Check if build behaves as expected |
|
@declension I think this is very close. Please also have a look at the hidden comments. They aren't much. Especially: #3052 (comment) |
|
Ok. I'm still not fully sure about the column length. It's a bit unfortunate that we need to have different ones in Python and C files - as far as I understand. It's certainly not blocking. I'll read through the code once again, but I think that after the commits have been squashed into one, we can merge this. |
Is this to the maintainer or to me? I normally leave this to maintainers since it's more about their choice than the contributors, but if you want me to force push something I can I guess |
|
If you squash it authorship is preserved in a better way. So I'd prefer you doing it. |
ann0see
force-pushed
the
add-python-linting
branch
2 times, most recently
from
8063772
to
b28e22e
Compare
* Ended up bigger than I'd hoped, but I think these are nearly all good changes... * Add pylint in CI * Split other linters into own jobs, was getting a bit all-or-nothing in that job * Add a sane config tweaked from default, but less onerous * Define the least-delta max-line-length that PEP-008 allows, 99 chars, and reflow a few lines. Looks nice anyway IMO * Fix all the various errors * Fix a few typos etc too * Rename one module (script) that wasn't python-friendly (snake_case) naming * ...and references to it * Initialise some object attributes in the constructor, not randomly elsewhere (soft warning in IDE) This actually found a few bugs, and gotchas (woohoo) so it'd be good if someone could sanity check those changes here: * In get_release_contributors `realname` was never defined so that code path would throw exceptions * Some of the regex char classes were actually control characters (missing the raw string prefix), so fixed. This closes jamulussoftware#2546 Actually remove all escaping of literal braces Simple testing proves this isn't needed here: $ cat /usr/include/qt/QtCore/qlocale.h | python -c 'import re; import sys; s = sys.stdin.read(); print(re.search(r"enum Country[^\n]+{([^}]+)}", s).group(1))' | md5sum 46625fcf574732033f70b2f10ce25bc4 - vs $ cat /usr/include/qt/QtCore/qlocale.h | python -c 'import re; import sys; s = sys.stdin.read(); print(re.search(r"enum Country[^\n]+\{([^}]+)\}", s).group(1))' | md5sum 46625fcf574732033f70b2f10ce25bc4 -
ann0see
force-pushed
the
add-python-linting
branch
from
b28e22e
to
0fd01a0
Compare
|
Congrats - and sorry for the huge delay. But development on Jamulus is slow... |
|
Codacy on my repo still found an error https://app.codacy.com/gh/ann0see/jamulus/pullRequest?prid=11754631 I believe it's not that serious |
Yes that looks safe to me - as it says there are no shell injection possibilities here (no shell at all), and I can't currently see any way that passing an |
|
Thanks for merging, and all your reviews @ann0see! |
Short description of changes
ℹ️ This actually found a few bugs / gotchas I think (woohoo it was worth it) - so it'd be good if someone could sanity check those changes here, especially:
realnamewas never defined so that code path would throw exceptionsCHANGELOG: Autobuild: Modernise / fix some Python and add linting to CI
Context: Fixes an issue?
Fixes: #2546
Does this change need documentation? What needs to be documented and how?
Probably not?
Status of this Pull Request
What is missing until this pull request can be merged?
Checklist
Linting results on current
mainAs a reference, these are the issues that currently come up on
main(at 20148a7)