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

ci/lint: add python linting #14848

Merged
merged 8 commits into from
Oct 6, 2024
Merged

Conversation

nathanruiz
Copy link
Contributor

Enabled the ruff python linter in CI, and resolved the issues that were detected by it. Ruff was used due to it's faster checking compared with other linters like pylint. This was added to resolve the python linting request in #13608.

Copy link

github-actions bot commented Sep 14, 2024

Download the artifacts for this pull request:

Windows
macOS

@kasper93
Copy link
Contributor

Interesting. Not much changes overall. I think it is pretty tamed by default. Probably to make it useful, some more checks could be enabled. Not saying to be pedantic, but some minor things like mixed single and double string quotes, line length, some more common checks in python world, would probably be useful. We don't have much python code, so not really makes much difference either way, but might as well improve what we have.

@nathanruiz
Copy link
Contributor Author

@kasper93 I've add a number of common checks that seem helpful:

  • pyflakes and pycodestyle checks - this includes a number of standard python checks, including line too long
  • pyflake8 standard, comma, and quotes - more standard checks, and checks for mixing of double and single quotes
  • pep8-naming - pep8 naming convention checking
  • pyupgrade - checking for code that could be written better with newer python features

@kasper93
Copy link
Contributor

kasper93 commented Sep 21, 2024

Looks quite complete now, thanks. There are remaining issues with scripts though, look up the CI result for them.

@llyyr
Copy link
Contributor

llyyr commented Sep 21, 2024

Could this be adjusted to only run when a file with *.py is modified? Feels wasteful to run this everytime a PR is submitted

@nathanruiz
Copy link
Contributor Author

@llyyr I looked into this briefly, but it seems like you can only exclude workflow execution based on files changed, not the jobs within. I'm happy to split it out into it's own workflow, but I wasn't sure if that is what you intended. I should also mention that the workflow took 7 seconds last time, so it may not have a huge impact.

TOOLS/dylib_unhell.py Outdated Show resolved Hide resolved
TOOLS/dylib_unhell.py Show resolved Hide resolved
@Akemi
Copy link
Member

Akemi commented Sep 29, 2024

not sure about - Restrict line length to 88 character, since project wide we have a 80/100 limit. though i guess it's explicitly for c files?

your last commit ci/lint: fix macos dylib_unhell.py build issue should be part of ci/lint: replace format strings where the error was introduced. also this worries me that other small breakages were introduced because of the linting.

[edit]
if the commit messages don't describe what was actually done in those commits, or rather other things were changed with them, they are kinda pointless. also mixing actual code/procedural changes with styling changes are quite dangerous and really hard to spot in all those changes.

@kasper93
Copy link
Contributor

not sure about - Restrict line length to 88 character, since project wide we have a 80/100 limit. though i guess it's explicitly for c files?

I agree on this point, we should use 100 limit in linters, while trying to keep it in 80 range, no point in forcing it if it looks worse.

Enabled the ruff python linter in CI, and resolved the issues that were
detected by it. Ruff was used due to it's faster checking compared with
other linters like pylint. This was added to resolve the python linting
request in mpv-player#13608.
This change required some minor rework to make the code conform to the
following:
- Restrict line length to 88 character
- Use spaces rather than tabs (only affect ci/lint-commit-msg.py)
- Use f-strings rather than % formatting or `str.format()`
@nathanruiz
Copy link
Contributor Author

not sure about - Restrict line length to 88 character, since project wide we have a 80/100 limit. though i guess it's explicitly for c files?

I've updated the linter to only check for 100 characters per line.

@Akemi
Copy link
Member

Akemi commented Oct 5, 2024

thanks for the changes and fixes. i quickly tested the macOS specific changes and the script still produce what is expected.

@kasper93
Copy link
Contributor

kasper93 commented Oct 6, 2024

I see no point to keep it in limbo, let's iterate on master if needed.

@kasper93 kasper93 merged commit 62abcbd into mpv-player:master Oct 6, 2024
25 checks passed
@nathanruiz nathanruiz deleted the python-linting branch October 11, 2024 21:04
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

Successfully merging this pull request may close these issues.

4 participants