Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

github: added explicit do not merge label to label check#29494

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
story645 wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromstory645:dnm

Conversation

story645
Copy link
Member

PR summary

Since we already run a check that blocks merging on certain labels, added a specific "do not merge" for situations where it doesn't fit the other do not merge categories.

PR checklist

@timhoffm
Copy link
Member

Aren'tdraft pull requests enough?

Draft pull requests cannot be merged, and code owners are not automatically requested to review draft pull requests.

Since I believe we do not care about code owners, effectivelydraft is equivalent todo not merge. Am I overlooking something?

jklymak reacted with thumbs up emoji

@story645
Copy link
MemberAuthor

Aren'tdraft pull requests enough?

Should be, but you also had a very explicit DO NOT MERGE on#29493 on top so I figured since we already had the infrastructure it wasn't gonna cost anything to add it?

@timhoffm
Copy link
Member

I did that explicit DO NOT MERGE because I was not sure whether full CI was running on draft PRs and thus whether I could use them . Since I know now that they are, I will now only usedraft without further comment/tag.

I find the draft mode better than a tag. The draft mode is an intention not to merge, but I can still see whether the technical requirements are met. The tag would always fail the corresponding action and it’s more cumbersome to dig out whether it failed due to the tag only or whether there are other technical blockers.

@story645
Copy link
MemberAuthor

I find the draft mode better than a tag.

I agree - I'm all for using the more precise/specific option and then falling back.

You're also not the only person I've seen put "Do not merge" on things that weren't drafts. All this was intended as was a fallback when "waiting for upstream" or "needs discussion" doesn't quite fit and also they don't want it in drafts.

But also yeah not attached to this PR so I'll eventually close it if nobody would use the tag.

@scottshambaugh
Copy link
Contributor

scottshambaugh commentedJan 30, 2025
edited
Loading

I think this is also useful for maintainers to be able to block PRs from contributors who might not know how to handle drafts, with the flexibility for other maintainers to release it. "Request changes" fulfills this to some extent, but requires that the original maintainer be the one to remove the block.

story645 reacted with thumbs up emoji

@jklymak
Copy link
Member

I'm not sure either of those are actually issues. Maintainers can dismiss a blocked review from another maintainer. They can also change the draft status of a PR.

timhoffm reacted with thumbs up emoji

@story645
Copy link
MemberAuthor

story645 commentedJan 30, 2025
edited
Loading

Maintainers can dismiss a blocked review from another maintainer.

Like I understand many folks don't see it this way, but for me dismissing someone elses block feels disrespectful so I'd feel uncomfortable doing so w/o their approval. Unless the block was a straight technical list of changes,

They can also change the draft status of a PR.

Also something I'd feel uncomfortable doing for someone else 'cause I don't want to assume I know what they intended. Especially since we've been wildly inconsistent on "are drafts reviewable?" I'd be more likely to leave a comment "hey, you may want to make this a draft"

Which the point of this PR was a fallback label that could always be replaced with something more precise.

@jklymak
Copy link
Member

Etiquette aside, the mechanics of this label are identical to the mechanics of draft status or a blocking/unblocking a review. The same etiquette would apply to adding or removing the label, I'd think

@story645
Copy link
MemberAuthor

story645 commentedJan 30, 2025
edited
Loading

The same etiquette would apply to adding or removing the label, I'd think

I figure people would explain why something is labeled do not merge > and actually I just got my use case#27304 where it would have been very useful to prevent the merging while I was in the middle of working on what was a longish review - it definitely doesn't belong in drafts but I also don't know if the review is blocking w/o finishing it.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@story645@timhoffm@scottshambaugh@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp