Add default_enabled option to optional message dict
#7629
Conversation
Pull Request Test Coverage Report for Build 3264705099
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
|
This feels like it fixes part of #3512 but not fully. I'm -0,5 for adding any more complexity to enabling and disabling that doesn't immediately solve that issue. It just creates more to consider before we implement the system discussed there. |
There was a problem hiding this comment.
I think it's a good idea, we definitely need an easy way to make a check optional (beside moving code to another directory and adding or removing code during registration, that's barbaric 😄). We need a better way but plugin maintainer also need a better way imo.
I would use a positive formulation for the option name to avoid a double negative (disabled = False => enabled = True), maybe default_enabled or enabled_by_default ?
cdce8p
changed the title
default_disabled option to optional message dictdefault_enabled option to optional message dict
There was a problem hiding this comment.
I think it's a good idea, we definitely need an easy way to make a check optional (beside moving code to another directory and adding or removing code during registration, that's barbaric 😄). We need a better way but plugin maintainer also need a better way imo.
I think we should be careful when considering if a check should be disabled by default. It's much easier for new ones and extensions. However, I do believe there is at least the expectation that all default checks are also enabled.
That doesn't negate that having an easy option to do so would be useful IMO.
I would use a positive formulation for the option name to avoid a double negative (disabled = False => enabled = True), maybe
default_enabledorenabled_by_default?
Done
Sure, definitely something that will help for #3512 but should not change much after that. |
|
But don't want we want to plan out that PR and then work based on that design? We're not sure how this would interact with the extending/disabling that is proposed in that issue. |
|
I'm pretty sure a boolean flag on each message is the simplest / most extensible / scalable way to disable / enable messages. Actually I've wanted to do that for a long time, (thank you @cdce8p). But I'm open to another design if you have one Daniel |
|
I don't have a design for the final implementation, but I think it makes sense to consider where the information is needed. If we go down a templating route it might make more sense to store such information in a default template for example. I'm just trying to avoid double work later on. |
|
Could you explain what you meant and the advantages of the template approach @DanielNoord ? I'm not sure about what it mean. |
|
Well just the whole discussion in that issue: The implementation of those questions might be more complicated if we add more stuff right now. |
|
Ha thank you I get it know. I think this MR won't impact the shareable template configuration, this is only a way for pylint to internally change our default values easily. I think we should not expose the |
I agree. The way I think about it, there are / will be multiple layers:
Since |
|
Yeah, but it might interact with how we implement |
|
I think the separation of default configuration / templated configuration / user overwrite will prevent most issue with this, I'll let you merge if you agree @cdce8p. |
I agree. It's good to be careful but I don't see an issue with |
Description
Add a new
default_enabledoption to make it easier to disable a specific message by default.This is especially useful for extensions like the
CodeStyleone. I'm not so sure if we should use it for regular checks though. The expectation I believe is that all of them are enabled by default. I.e. to disable one, we should move it to an extension first.