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

Disable design by default #3650

Closed

Conversation

danrneal
Copy link
Contributor

Description

This PR disables the design checker by default. The only time I have seen these messages be used effectively, has been when the arbitrary default parameters are configured to a team/individual's liking. It follows that if a user is savvy enough to configure the design parameters, they are savvy enough to enable the checker. However, for someone who is looking for a pip-and-play option, these messages can be a nuisance.

The design checker in my opinion should not be removed from pylint because they can be powerful when configured, but without proper configuration (which is highly individualistic), they should be disabled.

Type of Changes

Type
🔨 Refactoring
📜 Docs

Related Issue

Starts to address #3512

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.

Thanks for the merge request. I'm going to wait for another reviewer opinion before merging.

.gitignore Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 90.688% when pulling bc9e78e on danrneal:disable-design-by-default into 1ad2f25 on PyCQA:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 90.688% when pulling bc9e78e on danrneal:disable-design-by-default into 1ad2f25 on PyCQA:master.

@coveralls
Copy link

coveralls commented May 28, 2020

Coverage Status

Coverage increased (+0.001%) to 90.724% when pulling e1deda5 on danrneal:disable-design-by-default into e602fda on PyCQA:master.

Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @danrneal !
Honestly i'm a bit ill at ease with it because there is a discussion about how to improve pylint (#746) and one of the item concerns the default message system.
I definitely think that deactivating some default messages is what we need but maybe deactivating a whole module is a bit too strong. @AWhetter , @PCManticore, @Pierre-Sassoulas what do you think about it?

doc/whatsnew/2.6.rst Outdated Show resolved Hide resolved
@danrneal
Copy link
Contributor Author

I know not everyone agrees with me but my point of view has always been that there are no 'sane' defaults for design parameters as those are very org/project dependent and need to be tuned to be of any use. Those who are savvy enough to tune them are savvy enough to turn them on as well. However, those looking for a plug-and-play works-out-of-the-box linter, will get only frustration from this module.

I do think it is a good addition to add documentation as to how to turn it on and will add that! Thanks!

@danrneal danrneal force-pushed the disable-design-by-default branch from bc9e78e to e1deda5 Compare September 20, 2020 20:02
@hippo91
Copy link
Contributor

hippo91 commented Oct 1, 2020

@danrneal thanks for this. I'm ok with it, and find it's a good idea. I would like to have opinions of @AWhetter @PCManticore and @Pierre-Sassoulas about it before merging.

@Pierre-Sassoulas
Copy link
Member

I think @PCManticore gave his opinion here, this is probably best discussed in the #3512 directly.

@AWhetter
Copy link
Contributor

AWhetter commented Oct 3, 2020

I feel like we're jumping the gun a bit. We haven't really reached a consensuses on #3512 yet. We're not sure exactly which messages we want disabled by default, how we want to disable them and allow re-enabling, and whether we need to consider doing a major version bump for this change.
I would prefer for us to finish these design discussions first (although I'm aware of the lack of activity on #3512). I think it's less important to know exactly which messages we want enabled or disabled by default because I think it's likely to change over time.

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.

Removing approval as consensus is not reached.

@hippo91
Copy link
Contributor

hippo91 commented Oct 3, 2020

@Pierre-Sassoulas @AWhetter thanks for your answer. Let's wait for a consensus on #3512 so.

@Pierre-Sassoulas Pierre-Sassoulas added Blocked 🚧 Blocked by a particular issue Discussion 🤔 labels Dec 31, 2020
@Pierre-Sassoulas
Copy link
Member

Closing because of inactivity.

@DanielNoord DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants