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

Automatically apply "skip news" when detecting a docs-only change? #457

Closed
srittau opened this issue May 17, 2022 · 13 comments · Fixed by #485
Closed

Automatically apply "skip news" when detecting a docs-only change? #457

srittau opened this issue May 17, 2022 · 13 comments · Fixed by #485

Comments

@srittau
Copy link

srittau commented May 17, 2022

I noticed this in python/cpython#92877: bedevere seems to detect a docs-only change as it applies the "docs" label. But the bedevere/news step still fails as "skip news" is not applied. As docs changes don't need NEWS entries, maybe it would make sense to apply this label automatically and don't the the action step? But maybe there are cases that still require a NEWS entry and that would be missed with this change?

@AlexWaygood
Copy link
Member

AlexWaygood commented May 17, 2022

The difficulty is that (according to the current policy as I understand it), some docs changes do require news entries. E.g. if you're adding a whole new section to the docs, or documenting a previously undocumented class or function, that's probably newsworthy.

I agree that the current situation is annoying, though, since the vast majority of docs PRs we get do not need news entries.

@hugovk
Copy link
Member

hugovk commented May 17, 2022

The devguide says of "skip news":

Similar to the skip issue label, this label is used for PRs which involve trivial changes, backports, or already have a relevant news entry in another PR. Any potentially impactful changes should have a corresponding news entry, but for trivial changes it’s commonly at the discretion of the PR author if they wish to opt-out of making one.

Keywords are trivial and opt-out.

@srittau
Copy link
Author

srittau commented May 17, 2022

But a different section of the devguide says:

[...] Changes that affect documentation only generally do not require a NEWS entry.

@AA-Turner
Copy link
Member

AA-Turner commented May 17, 2022

The earliest application of the "skip news" label on a PR with "docs" was on 18 Aug 2017.

Since then, 648 PRs have been labelled "docs", of which 543 (84%) have had both labels.

This indicates to me that applying "skip news" automatically and leaving it to triagers/core developers to remove seems the option that would save the most time.

A


Query for "docs":

https://github.com/python/cpython/pulls?q=is%3Apr+created%3A%3E%3D2017-08-18+label%3Adocs

Query for "docs" and "skip news":

https://github.com/python/cpython/pulls?q=is%3Apr+created%3A%3E%3D2017-08-18+label%3Adocs+label%3A%22skip+news%22

@srittau
Copy link
Author

srittau commented May 17, 2022

@AA-Turner Thanks for collecting those stats!

And just to clarify: I have no stake in this as I'm neither a core dev nor a triager. It's just something I noticed that might ease work for those groups and possibly be a bit friendlier to new committers. If you decide that it's a bad idea, that's perfectly fine by me.

@AA-Turner
Copy link
Member

AA-Turner commented May 17, 2022

There was a small error in my summary as the 17 May 2017 PR had the "skip news" label applied this month. I've updated my post, although the rounded percentage is the same.

I'm in favour of the change, to be clear. We could post on Discourse for a wider audience, although I'm not sure that it would be that controversial.

A

@AlexWaygood
Copy link
Member

We could also consider tweaking Bedevere's comments like these so that he doesn't post them on docs-only PRs from first-time contributors

@AA-Turner
Copy link
Member

I'd argue it'd be simpler in the logic to just skip the news check altogether by default -- I think it would be odd to not have the message on your first documentation PR, and then to be chastised on your second as you are no longer considered a first-time contributor.

From a quick look at Bedevere's code, it seems we'd need to change prtype.py to also apply "skip news" and filepaths.py to switch the precedence of the news and classification checks.

A

@AlexWaygood
Copy link
Member

AlexWaygood commented May 17, 2022

and then to be chastised on your second as you are no longer considered a first-time contributor.

He only ever comments on PRs by first-time contributors -- it is assumed that second-time contributors know how to add NEWS entries :)

But I agree that the better course is probably just to get him to add "skip news" to all docs-only contributions. This is just me floating a less radical alternative, if people object to that.

@AA-Turner
Copy link
Member

Ahh, I retract my comment then. Sorry!

A

@ezio-melotti
Copy link
Member

The proposal seems reasonable to me. If the number of changed lines is readily available we could still apply the label for major changes, but otherwise skipping the label and relying on triagers/reviewers to add it when needed should be ok.

We could also consider tweaking Bedevere's python/cpython#92754 (comment) so that he doesn't post them on docs-only PRs from first-time contributors

FWIW the content of the comment was changed in #447.

@ezio-melotti
Copy link
Member

@kj7rrv
Copy link

kj7rrv commented Jul 7, 2022

If adding a new section to the docs is the only common case where a news entry is required, would it be possible to automatically detect that?

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

Successfully merging a pull request may close this issue.

6 participants