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

Incrementally enable new Pylint lints #9614

Open
Eric-Arellano opened this issue Feb 17, 2023 · 4 comments
Open

Incrementally enable new Pylint lints #9614

Eric-Arellano opened this issue Feb 17, 2023 · 4 comments
Assignees
Labels
refactor Proposal to refactor code

Comments

@Eric-Arellano
Copy link
Collaborator

Eric-Arellano commented Feb 17, 2023

Our previous Pylint version was 2.8, and I recently upgraded it to 2.16 in 041877b. To facilitate landing it, I disabled a bunch of checks. But most of them would be good to activate.

Working on this PR means:

  1. Choose one of the checks you want to enable from here:

https://github.com/Qiskit/qiskit-terra/blob/4e3b283157b43687db4260cee6decf17fbb37608/.pylintrc#L77-L109

  1. Delete it from .pylintrc
  2. Run tox -e lint and fix all the issues until tox -e lint succeeds.

To make it easier for code reviewers, it's a good idea to activate 1 or 2 checks at a time.

@Eric-Arellano Eric-Arellano added help wanted community contributions welcome. For filters like http://github-help-wanted.com/ refactor Proposal to refactor code labels Feb 17, 2023
@Eric-Arellano Eric-Arellano self-assigned this Feb 17, 2023
@github-project-automation github-project-automation bot moved this to Tagged but unassigned in Contributor Monitoring Feb 18, 2023
@javabster javabster moved this from Tagged but unassigned to Assigned in Contributor Monitoring Feb 24, 2023
@Eric-Arellano Eric-Arellano removed the help wanted community contributions welcome. For filters like http://github-help-wanted.com/ label May 16, 2023
@lambda-knight
Copy link

Fixing consider-using-f-string (C0209) is worth trying ?

@Eric-Arellano
Copy link
Collaborator Author

Yes, @lambda-knight consider-using-f-string looks useful to me. Note though that we are transitioning to Ruff instead of Pylint. So it may be worth double checking that Ruff has a similar lint.

Even if Ruff doesn't have it, I still think it's valuable to fix all our historical instances regardless of if we introduce future "regressions" or not. Let's say we have 200 issues now, and we introduce 50 in the next 60 months. Better to have 200/250 fixed than 0/250.

@lambda-knight
Copy link

OK, @Eric-Arellano ,
Enabling consider-using-f-string, 192 files and 520 warnings are reported. I am fixing code of this warning and will issue PR.

@joesho112358
Copy link
Contributor

@lambda-knight hi, can i help out with consider-using-f-string?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Proposal to refactor code
Projects
Status: Assigned
Development

No branches or pull requests

3 participants