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

Pylint fixes for master branch #3054

Merged
merged 7 commits into from
Jan 7, 2025
Merged

Pylint fixes for master branch #3054

merged 7 commits into from
Jan 7, 2025

Conversation

Lestropie
Copy link
Member

#3052 is failing CI for multiple reasons. One of them is errors during pylint testing, presumably due to the pylint version on the Action being incremented. Here I have addressed those issues on master, as well as some other Errors that pylint raised on my own system when checking master.

Some changes are due to warnings about reading from uninitialised variables, which arise due to using a "elif variable == 'string'" substitute-for-switch pattern that is not guaranteed to be exhaustive from the perspective of the linter. I tend to prefer to keep that pattern rather than using an "else:" for the last option as it's more robust to future augmentation.

I have not addressed the warning about distutils being deprecated.

On my local system I also get a lot of these, will wait to see if GitHub complains about it also:

pylint: Command line or configuration file:1: UserWarning: 'Exception' is not a proper value for the 'overgeneral-exceptions' option. Use fully qualified name (maybe 'builtins.Exception' ?) instead. This will cease to be checked at runtime when the configuration upgrader is released.
************* Module testing/pylint.rc
testing/pylint.rc:1:0: E0015: Unrecognized option found: argument-name-hint, attr-name-hint, class-attribute-name-hint, class-name-hint, const-name-hint, function-name-hint, inlinevar-name-hint, method-name-hint, module-name-hint, variable-name-hint, no-space-check (unrecognized-option)

- Do not attempt to install distutils in CI.
- In all locations where either shutil.which() or distutils.spawn.find_executable() is used, first attempt to import shutil.which(), and disable pylint deprecated module check on the distutils import.
- Locally disable too-many-positional-arguments() on a few functions. Those in "configure" will be deprecated in 3.1.0; those in "population_template" can have a more suitable fix implemented on the development branch, but for "master" branch for now the check will simply be bypassed.
- pylint.rc:
  - Update to reflect changes to names and expected values of those settings relating to object naming.
  - Disable deprecated no-space-check.
  - Fix overgeneral-exceptions.
Module is only loaded in the absence of shlex.quote().
This import syntax should ideally be removed entirely upon merge to development branch for inclusion in 3.1.0.
Package "python2" no longer available in GitHub CI Action.
@Lestropie
Copy link
Member Author

While c5c41a8 is unrelated to pylint, it is requisite to pass the Linux Clang CI test. If proposed separately it would not be possible to merge either set of changes onto master independently as neither would pass all tests. While it would be better to create two separate PRs for the two separate changes and then construct a PR containing both sets of changes that could actually be merged, I don't think it's worth the effort in this particular instance, especially given it's holiday season.

daljit46
daljit46 previously approved these changes Jan 2, 2025
@Lestropie Lestropie added this pull request to the merge queue Jan 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2025
@Lestropie
Copy link
Member Author

Requires a renewed review; merge failed due to needing to re-run update_copyright.

daljit46
daljit46 previously approved these changes Jan 6, 2025
@Lestropie
Copy link
Member Author

Sorry, need another one; forgot to run generate_user_docs.sh after update_copyright :-/

@Lestropie Lestropie added this pull request to the merge queue Jan 7, 2025
Merged via the queue into master with commit 4040c17 Jan 7, 2025
5 checks passed
@Lestropie Lestropie deleted the pylint_master branch January 7, 2025 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants