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

Add docs label for PRs that touch Doc/tools/.nitignore #616

Closed
skirpichev opened this issue Jan 19, 2024 · 9 comments · Fixed by #617
Closed

Add docs label for PRs that touch Doc/tools/.nitignore #616

skirpichev opened this issue Jan 19, 2024 · 9 comments · Fixed by #617

Comments

@skirpichev
Copy link
Member

See e.g. python/cpython#114280 or python/cpython#114194

@hugovk hugovk changed the title bedevere could add docs label for prs, that touch Doc/tools/.nitignore Add docs label for PRs that touch Doc/tools/.nitignore Jan 19, 2024
@hugovk
Copy link
Member

hugovk commented Jan 19, 2024

Good idea!

I found it a bit annoying the label is added for PRs touching rst files in Docs/, but not those touching rst plus .nitignore.

@skirpichev
Copy link
Member Author

PR: #617

@ezio-melotti
Copy link
Member

I found it a bit annoying the label is added for PRs touching rst files in Docs/, but not those touching rst plus .nitignore.

Why does this happen? Isn't bedevere already adding the docs label whenever at least an .rst file has been changed, regardless of the other files?

@skirpichev
Copy link
Member Author

Looking in the sources, it doesn't add any labels (pr_labels == []) if there is some file, which doesn't match known patterns:

for filename in filenames:
if util.is_news_dir(filename):
news = True
filepath = pathlib.PurePath(filename)
if filepath.suffix == ".rst":
docs = True
elif filepath.name.startswith("test_"):
tests = True
else:
return pr_labels

Another case: #605

@ezio-melotti
Copy link
Member

So the problem is the return (prematurely?) terminating the loop? Is that intentional?

@skirpichev
Copy link
Member Author

Is that intentional?

I guess - so. Or every pr that has some *.rst file could skip news.

@AlexWaygood
Copy link
Member

Is that intentional?

I guess - so. Or every pr that has some *.rst file could skip news.

That wouldn't be correct — there are lots of PRs that introduce new features and add docs for the new feature as part of the same PR. Most of those PRs need news entries.

@skirpichev
Copy link
Member Author

(On another hand, most PRs, that touch only test files - probably could skip news;) That's another story, however.)

@ezio-melotti
Copy link
Member

Ok, so the goal is to add the docs label only to PRs that are "docs-only", i.e. they only change documentation and possibly files related to the documentation (like .nitignore). If a PR adds a new feature and edits the docs to document it, it's not "docs-only" and shouldn't get the docs label.

(Maybe this should be spelled out clearly in a comment/docstring, including the expected behavior for other labels, since the intention is currently not too clear IMHO).

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

Successfully merging a pull request may close this issue.

4 participants