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

allow-global-unused-variables should respect dummy-variables-rgx #9570

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

com6056
Copy link

@com6056 com6056 commented Apr 25, 2024

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Right now, --allow-global-unused-variables=no doesn't respect --dummy-variables-rgx, I believe this should fix it 🀞

Refs #8174

Closes #8174

This comment has been minimized.

@com6056
Copy link
Author

com6056 commented Apr 26, 2024

Not sure if this change requires news or not, let me know if it does though! I just don't have all the tools installed to regenerate everything but can get that going if needed.

@DanielNoord
Copy link
Collaborator

This would indeed require a news entry, if you don't want to install the tools you can probably copy one from a previous PR and see what the syntax looks like there.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.81%. Comparing base (67bfab4) to head (e80d0a5).
Report is 10 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9570      +/-   ##
==========================================
- Coverage   95.81%   95.81%   -0.01%     
==========================================
  Files         173      173              
  Lines       18825    18827       +2     
==========================================
+ Hits        18038    18039       +1     
- Misses        787      788       +1     
Files Coverage Ξ”
pylint/checkers/variables.py 97.28% <50.00%> (-0.07%) ⬇️

@com6056
Copy link
Author

com6056 commented Apr 29, 2024

This would indeed require a news entry, if you don't want to install the tools you can probably copy one from a previous PR and see what the syntax looks like there.

Done! Just peeked at some other PRs, took a few tries but seems like it is happy now haha, let me know if there's anything else needed before we can get this merged in! πŸŽ‰

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for opening a PR. I do think we need to think about #8013 before "fixing" this. There's a bad/ "two options in parallel" initial design and we need to restore consistency (and document this better then close #8013 in this MR, if this MR restore consistency).

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.2.0 milestone Apr 29, 2024
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Apr 29, 2024
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit e80d0a5

@com6056
Copy link
Author

com6056 commented Apr 29, 2024

Thank you for opening a PR. I do think we need to think about #8013 before "fixing" this. There's a bad/ "two options in parallel" initial design and we need to restore consistency (and document this better then close #8013 in this MR, if this MR restore consistency).

Any idea how long this will take? Could I instead just add a new flag that allows you instead to opt-in to this behavior then if it might take a while to resolve #8013?

@Pierre-Sassoulas
Copy link
Member

If you can propose a consistent design it could be fast, otherwise we need to wait for someone else to think this is important and volunteer to think about it. Maybe the current MR is enough and making them mutually exclusive and using only one option at a time is not the way to go. But it should be argumented/documented. Right now I feel we're fixing a particular use case, instead of actually fixing the root issue and are going to throw all this when actually looking into it.

@com6056
Copy link
Author

com6056 commented May 3, 2024

Where in the docs should I add this? And what do you mean by "argumented"? Should I add another CLI flag that enables this?

@Pierre-Sassoulas
Copy link
Member

And what do you mean by "argumented"? Should I add another CLI flag that enables this?

Sorry I was thinking in french, I actually meant argued / reasoned that the better solution is to keep both options and not make them mutually exclusive. We would need to document both of the options if that's the case (here

"help": "A regular expression matching the name of dummy "
).

@jacobtylerwalls jacobtylerwalls modified the milestones: 3.2.0, 3.3.0 May 7, 2024
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.3.0, 3.4.0 Sep 20, 2024
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.4.0, 4.0.0 Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow-global-unused-variables doesn't respect dummy-variables-rgx
4 participants