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

Lock Major Change Proposal issue #1858

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Lock Major Change Proposal issue #1858

merged 2 commits into from
Dec 9, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Nov 18, 2024

This PR makes it so that every new major change proposal issue is locked by default.

This is done to affirm more strongly that discussion should happen on the generated Zulip thread instead.

This is particularly relevant for outsiders who (I think) just skip the part about no technical discussion on the issue, as it will systemically prevent them from doing so.

The locking of the issue doesn't affect "collaborators", aka members of the repo with at least "write" access (which is the case for all t-compiler/t-types members).

cc @apiraino
r? @ehuss

@apiraino
Copy link
Contributor

... The locking of the issue doesn't affect "collaborators"

... and our bots as well, I imagine. Correct?

I am more or less neutral on this change, my remembering is we don't have very often people discussing on the issue itself and ignoring Zulip. On the other end, why not 🙂

@Urgau
Copy link
Member Author

Urgau commented Nov 19, 2024

... and our bots as well, I imagine. Correct?

Yes, they also get "write" permissions.

[..] my remembering is we don't have very often people discussing on the issue itself and ignoring Zulip.

I agree it's not very recurring but when it happens we are put in a bad state where the comment will likely be missed and if it isn't missed we have to remind people to post it on Zulip which IMO greats a bad taste (especially for newcomers). It greats a disconnect between the two place, which this PR makes it much harder to happen for IMO a small cost.

@apiraino

This comment was marked as off-topic.

Copy link
Contributor

@apiraino apiraino left a comment

Choose a reason for hiding this comment

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

left some comments for discussion

src/handlers/major_change.rs Show resolved Hide resolved
@@ -416,6 +416,18 @@ pub enum ReportedContentClassifiers {
Spam,
}

#[derive(Debug, serde::Deserialize, serde::Serialize, Eq, PartialEq)]
pub enum LockReason {
Copy link
Contributor

@apiraino apiraino Nov 22, 2024

Choose a reason for hiding this comment

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

Which of these reasons would apply to the use case "tracking issues are just not for discussions"?

Copy link
Member Author

@Urgau Urgau Nov 23, 2024

Choose a reason for hiding this comment

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

None of these apply/transmit the idea we want, so I wouldn't use any of them. The reason is optional anyway, I didn't use any of them for this PR.

@apiraino
Copy link
Contributor

Ok, so personally I am in favor of experimenting this, it's just for the Compiler MCPs so a relatively small audience - which I think it's safe to assume is already mostly on Zulip, so probably my concern about "where else would a discussion take place" is unfounded.

I'll let T-compiler get a FIY about this.

@apiraino
Copy link
Contributor

apiraino commented Dec 9, 2024

Quick update: this was discussed in T-compiler triage meeting (on Zulip). The vibe is positive on testing this.

cc @ehuss when you have time for a quick look at this

thanks to both!

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit bfeafd5 into rust-lang:master Dec 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants