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

add pylint configuration and package #36

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Conversation

jurraca
Copy link
Contributor

@jurraca jurraca commented Nov 18, 2024

pylintrc generated with --generate-rcfile which outputs a ton of default config. I removed the empty settings and kept the stated defaults for the rest. Our disabled rules form CI are on line 209. Would it be better to remove the default config settings?

pylintrc generated with `--generate-rcfile` which outputs a ton of
default config. I removed the empty settings and kept the stated
defaults for the rest. Our disabled rules form CI are on line 209.
Copy link
Collaborator

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine for me to keep the defaults in but also fine to remove them, I have no strong opinion either way.

You should also update the CI pylint command to use the rc file in this PR so that the disablements are managed in one place.

pylintrc Outdated
too-many-statements,
too-few-public-methods,
fixme,
too-many-positional,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting this error here:

pylintrc:1:0: W0012: Unknown option value for '--disable', expected a valid pylint message and got 'too-many-positional' (unknown-option-value)

Copy link
Contributor Author

@jurraca jurraca Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what pylint version you running? Looks like nixpkgs is on 3.1.1 (fairly far behind) which has this option, but 4.0.0 3.3.1 onwards has too-many-positional-arguments arg. 4.0.0 3.3.1 would be the version we should follow in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nixpkgs 24.11 release is around the corner (currently in beta) and has 3.3.1. I'll test it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like I tested this outside of the nix shell and there I have pylint 3.3.1, so yeah, using that should fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated and tested locally with 3.3.1 👍

@fjahr
Copy link
Collaborator

fjahr commented Nov 28, 2024

tACK 8dd314d

Awesome, thanks!

@fjahr fjahr merged commit 6c934db into asmap:master Nov 28, 2024
1 check passed
@jurraca jurraca deleted the add-pylint branch November 28, 2024 19:07
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.

2 participants