Skip to content

Should bad-option-value be raised for moved or deleted messages? #6514

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

Closed
jacobtylerwalls opened this issue May 5, 2022 · 6 comments
Closed

Comments

@jacobtylerwalls
Copy link
Member

Question

Are we sure we want to emit bad-option-value in 2.14 for people's pylintrc files that have, say, no-self-use or star-args disabled? That seems a little strict to me, making people QA their config files.

This is new in 2.14 for config files, but not new in python code itself. I think we should discuss both. The config file is potentially worth revisiting during the beta.

This is more of a headache because there is no way to disable bad-option-value currently. See #3312 and in particular #3312 (comment). I think we're making this a bit worse with the 2.14 change.

Documentation for future user

We might consider not raising bad-option-value for deleted or moved-to-extension messages, just typos/parse errors. We already have constants for deleted messages.

Additional context

No response

@jacobtylerwalls jacobtylerwalls added Proposal 📨 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone May 5, 2022
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented May 5, 2022

I think it makes sense to now raise bad-option-value if the message exists but is not activated for this interpreter or configuration; We have a list of deleted messages in pylint.constant, we'd need to also have a list of message from unloaded extension or other interpreter than the one being used. It would work well with #5607 where we'd want to generate a structure containing the existing messages from pylint's code.

@DanielNoord
Copy link
Collaborator

I was not aware of the issues with disabling bad-option-value. That should indeed probably become a priority and potentially a blocker for 2.14.

That said, I think there is also some benefit to raising the message as it will show users that certain options or messages are no longer supported even if they thought they were. (For example, I found that homeassistant had unnecessary options in their config).
So, not raising on removed messages kinda defeats that purpose.

On the other hand, I agree that this might require more effort from users to upgrade to a newer version. I do think that updating a configuration file is a little less worse than updating your actual code so for me this would be allowable.

@Pierre-Sassoulas
Copy link
Member

On the other hand, I agree that this might require more effort from users to upgrade to a newer version

Once we have #5462 it becomes painless, until then let's not break user configurations.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Let's not put too much hope on #5462. That has now become an ini-to-toml-converting, missing-options-removing, interactive, context-aware autoupgrader... 😅

For me, being able to disable bad-option-value would be enough. For those who want to support various pylint versions with one version file they can do so, for others we keep warning.
Note that that would also be backwards compatible: by adding bad-option-value to disable we won't warn about it on 2.14 and on <2.14 we don't care about extra messages in disable=.

@jacobtylerwalls
Copy link
Member Author

Looking into #3312 during the beta sounds like a plan. Maybe we can leave this open for visibility during the beta if decide to pre-release as is.

@Pierre-Sassoulas Pierre-Sassoulas added Discussion 🤔 and removed Proposal 📨 Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much labels May 6, 2022
@DanielNoord
Copy link
Collaborator

See #3312 for a possible solution. Based on the emoji's I think we all agree that fixing #3312 before releasing 2.14 would be good enough to close this right? In that case I think we might as well close this and continue discussion there. That way people interested and subscribed to that topic don't get left out of any discussion around this topic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants