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

[NFC] Delete release branch PR template in favor of org-wide variant #2669

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

AnthonyLatsis
Copy link
Contributor

@AnthonyLatsis AnthonyLatsis commented Jun 4, 2024

swiftlang now has an equivalent organization-wide PR template, so we no longer need this local one.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you. Guess I now also need to update sourcekit-lsp again, where I just added the templates 😆 swiftlang/sourcekit-lsp@b844dd3

Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer not to have this in the pull request template. Almost all PRs, especially from contributors are targeting the main branch and so this doesn’t apply and would be noise.

Copy link
Contributor Author

@AnthonyLatsis AnthonyLatsis Jun 5, 2024

Choose a reason for hiding this comment

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

At your discretion. FWIW, the idea is to provide quick access to the form (as a workaround to not being able to automate the template selection).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside, I got curious and did the math: ~13% (swift-syntax) vs. ~43% (swift) PRs targeting release branches.

Copy link
Member

Choose a reason for hiding this comment

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

I think a major point for me is that it’s a different target audience. In my opinion the templates are particularly valuable for newcomers. But contributors contributing to release branches are usually seasoned.

How exactly did you measure that? 43% to release branches seems a little high to me. That basically means that every PR to main has a corresponding PR to a release branch, which can’t be the case since some time of the year there is not release branch to cherry-pick the changes to.

Copy link
Contributor Author

@AnthonyLatsis AnthonyLatsis Jun 5, 2024

Choose a reason for hiding this comment

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

Good judgement — I forgot about master. With master it is ~11% vs. ~17%.

Copy link
Member

Choose a reason for hiding this comment

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

OK, that makes a lot more sense. Thanks for clarifying. I stick with my opinion of not including it in the PR template though 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sure, I’m happy to take your word for what works best for swift-syntax. The numbers were not to prove a point, I just felt like sharing the info. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Those were really interesting numbers. I would hav personally expected them to be quite a bit lower. Showed that I was wrong 😄

CONTRIBUTING.md Outdated Show resolved Hide resolved
@AnthonyLatsis
Copy link
Contributor Author

Guess I now also need to update sourcekit-lsp again, where I just added the templates 😆 swiftlang/sourcekit-lsp@b844dd3

How is this issue template related, though? 😅

@AnthonyLatsis
Copy link
Contributor Author

How is this issue template related, though? 😅

Ah, I see, you meant this commit.

@AnthonyLatsis AnthonyLatsis changed the title [NFC] Link out to Swift.org for release branch PR form and process [NFC] Link out to Swift.org for release branch PR form and directions Jun 5, 2024
@AnthonyLatsis AnthonyLatsis changed the title [NFC] Link out to Swift.org for release branch PR form and directions [NFC] Link out to Swift.org for release branch PR directions Jun 5, 2024
@AnthonyLatsis AnthonyLatsis changed the title [NFC] Link out to Swift.org for release branch PR directions [NFC] Delete release branch PR template in favor of org-wide variant Jun 25, 2024
@AnthonyLatsis
Copy link
Contributor Author

I will amend the doc to point to the website once swiftlang/swift-org-website#688 is merged.

CONTRIBUTING.md Outdated
Comment on lines 92 to 95
```
https://github.com/swiftlang/swift-syntax/compare/main...my-branch?quick_pull=1&template=release.md
```
Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanna visual the change in the URL we can use diff code block here:

Suggested change
```
https://github.com/swiftlang/swift-syntax/compare/main...my-branch?quick_pull=1&template=release.md
```
```diff
- https://github.com/swiftlang/swift-syntax/compare/main...my-branch?quick_pull=1
+ https://github.com/swiftlang/swift-syntax/compare/main...my-branch?quick_pull=1&template=release.md

Not sure if that will be useful–it just come to my mind so adding a suggestion 🫶

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it a lovely idea. Thanks!

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Minor grammatical suggestion, otherwise looks good. Great that we can reference the pull request template from the .github repo!

CONTRIBUTING.md Outdated Show resolved Hide resolved
`swiftlang` now has an equivalent organization-wide PR template, so we
no longer need this local one.
@ahoppen
Copy link
Member

ahoppen commented Jun 26, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge June 26, 2024 05:14
@AnthonyLatsis
Copy link
Contributor Author

@swift-ci please test Windows

@ahoppen ahoppen merged commit 5c1adb9 into swiftlang:main Jun 26, 2024
3 checks passed
@AnthonyLatsis AnthonyLatsis deleted the prunus-persica branch July 12, 2024 00:27
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