-
Notifications
You must be signed in to change notification settings - Fork 343
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
Pylint update to 3.0.0 #5959
Pylint update to 3.0.0 #5959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the version bump, the spell check fails:
./selftests/spell.sh
** Running spell check...
No files to lint: exiting.
Thanks @clebergnu, I will investigate it. I am wondering why it wasn't caught on our CI. See https://github.com/avocado-framework/avocado/actions/runs/9549221339/job/26353906359?pr=5959#step:8:16 |
I looked into this, and I found that
As opposed to this which uses the "right" version:
IMO, |
Hello @clebergnu @richtja, I debugged a bit, pylint-3.0 introduced this commit: pylint-dev/pylint@d203de7 Spelling only check
|
Hi @PaulYuuu, thank you for your help. I will update the spellcheck command and the spell dict. |
Hi @clebergnu and @PaulYuuu, thank you for your reviews. I have force-pushed the fix for spell check. Please have a look. |
7db9433
to
a50d623
Compare
.github/workflows/ci.yml
Outdated
run: | | ||
export PATH=/github/home/.local/bin:$PATH | ||
python3 setup.py test --select=static-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @richtja, as we can ensure pylint will be installed during make requirements-dev
, can we modify those shell scripts to use tools directly like isort.sh/style.sh, without which pylint
, so we don't need this change I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use pylint-3
, seems it will be installed to /usr/bin/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PaulYuuu thanks for your help. I removed the pylint-3
because of this comment.
The reason for export
is that those scripts fails in CI without any logs. I am not sure why it fails with python3 -m pylint
therefore I'm moving this to draft, and I will add more verbosity to it to find out what is going on in CI environment.
e8026f6
to
613518e
Compare
Hi @clebergnu and @PaulYuuu, I did a couple of tests and at the end I decided to use only python module of pylint as the most stable solution. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but could you please rebase to latest master branch? I think there is a new word "secureboot" need to handle.
This will update the pylint to version 3.0.0 without enabling any new checks. This is needed because, the pylint 2.17.2 is not compatible with python3.12 Reference: avocado-framework#5957 Signed-off-by: Jan Richter <jarichte@redhat.com>
Avocado is using pylint-3 instead of default pylint. This is a leftover from the time when avocado supported python2. Unfortunately, the pylint-3 has slightly different behaviour than pylint, therefore we need to update it to have more accurate testing. Lets use python3 -m pylint Signed-off-by: Jan Richter <jarichte@redhat.com>
I will do. Thanks for catching that. |
The pylints spelling only check C0401 C0402 C0403, but `--errors-only` in the avocado spell check command line means pylint E rules only, therefore the avocado spell check is not working. This commit removes the `--errors-only` flag and fixes all spelling issues accumulated in avocado code. Signed-off-by: Jan Richter <jarichte@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This will update the pylint to version 3.0.0 without enabling any new checks. This is needed because, the pylint 2.17.2 is not compatible with python3.12
Reference: #5957