Take 'accept-no-raise-doc' option into account #7581
Conversation
Pull Request Test Coverage Report for Build 3339278684
💛 - Coveralls |
|
Is there a difference between this and #7212. We should probably focus on either of them. |
|
The difference is that, in my understanding, #7212 was changing the behavior of the option so that In this PR, |
I think it's better that way seeing the description of the |
| @@ -3,5 +3,6 @@ load-plugins = pylint.extensions.docparams | |||
|
|
|||
| [BASIC] | |||
| accept-no-param-doc=no | |||
| accept-no-raise-doc=no | |||
| @@ -1,2 +1,5 @@ | |||
| [MAIN] | |||
| load-plugins = pylint.extensions.docparams | |||
|
|
|||
| [BASIC] | |||
| accept-no-raise-doc = no | |||
There was a problem hiding this comment.
Another case of the breaking change.
Pierre-Sassoulas
added
Needs backport
There was a problem hiding this comment.
Change itself LGTM, just wondering about how to communicate this as clearly as possible. I agree with putting this in a minor release (not a patch release), but we should communicate very clearly as I could imagine it being annoying if you forget this change in 6 months time.
doc/whatsnew/fragments/7208.bugfix
Outdated
| @@ -0,0 +1,10 @@ | |||
| The ``accept-no-raise-doc`` option related to ``missing-raises-doc`` will now | |||
There was a problem hiding this comment.
@DudeNr33 @Pierre-Sassoulas Just thinking out loud: can we create a breaking_change category in towncrier so this fragments gets automatically placed at the top?
We don't need to call it breaking change as I don't think this actually is, so perhaps user_action_required or something. I think that would help avoid confusion.
There was a problem hiding this comment.
Yeah I think it would be very valuable to have a synthesis of change that user could need to do when upgrading. (for thing like no-self-use becoming optional and suddenly raising conf warning message, or a bug being fixed that means the behavior will change unexpectedly)
There was a problem hiding this comment.
Does towncrier support something like that out of the box?
There was a problem hiding this comment.
I checked the towncrier issues and it looks like it was supported in the past, but the latest version seems to have issues (see twisted/towncrier#437).
Will look into it if it works with the version we currently use.
There was a problem hiding this comment.
Sorry for the delay in review @bchardin. the changes themselves LGTM. Could you simply bring the change from main with a merge or a rebase and rename the fragment to user_action instead of bugfix now that it's possible, please ?
Defaut value for accept-no-raise-doc is True, but tests in `missing_doc_required_Sphinx.py` assume that this option is set to False.
for more information, see https://pre-commit.ci
|
I just changed the filename from |
|
I'm moving this to 2.16.0 as it's going to raise a lot of new messages seeing the primer and patches are not supposed to raise new messages. (Nothing to do on your end @bchardin it's just a maintenance decision). |
Pierre-Sassoulas
removed
the
Needs backport
Pierre-Sassoulas
added
Bug 🪲
False Negative 🦋
|
Congratulation on becoming a pylint contributor @bchardin 🎉 ! |
Type of Changes
Description
Previously, Pylint correctly raised
missing-raises-doc(W9006) when exceptions were raised by a function and:accept-no-raise-docbeing true or falseaccept-no-raise-docwas falseaccept-no-raise-docwas falseIt also incorrectly raised
missing-raises-docwhen:accept-no-raise-docwas trueThis PR fixes this last case, for which I believe
missing-raises-docshould not be raised.Closes #7208