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

meta: Add Showcase section to PR template #11750

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Feb 7, 2024

Objective

Oftentimes I find myself reading through a PR and not quite understanding what's going on. Even if it's super detailed, it can sometimes be difficult to imagine what the end result of the PR might look like.

For example, #10756 clearly communicates its goals and contains a descriptive Changelog. However, I was still a bit lost as to what a user might see from the change until I saw the dedicated example in the diff.

Solution

At the risk of giving contributors more work, I think a dedicated Showcase section could be really nice.

Along with providing reviewers stumbling on the PR with a "tangible summary" of the change, it should also help out when working on the release post. Sometimes someone other than the PR's author has to write up a blog section on the PR. This can be somewhat daunting to people wanting to contribute in that effort as they have to rely on the Migration Guide giving a decent example (assuming it's a breaking change), piecing together the other sections into a sensible example themselves, or manually reading through the diff.

Theoretically, this new Showcase section would be more of an encouragement than a strict requirement. And it's probably only going to be useful where there is something to showcase (e.g. visual changes, API changes, new features, etc.).

Bikeshedding

  • Naming. I also considered Demo and Example, but there may be others we prefer. I chose Showcase to communicate the feeling of fun and appreciation for the work contributors put in.
  • Position. I placed the section right above the Changelog section since I felt it made sense to move from the details in Solution to a brief example in Showcase to a tl;dr of the changes in Changelog
  • Phrasing. We can also bikeshed the bullet points and phrasing of each as well.

@MrGVSV MrGVSV added the A-Meta About the project itself label Feb 7, 2024
@BD103
Copy link
Member

BD103 commented Feb 9, 2024

Would it be worth mentioning in a comment that you can make a collapsible display, so the code does not take up a lot of screen space?

<details>
  <summary>Click to view showcase</summary>

<!-- Spaces added because markdown in markdown -->
` ` `rust
println!("My super cool code.");
` ` `

</details>
Click to view showcase
println!("My super cool code.");

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I agree with this section. There is some PR's that are almost only visual changes, but don't include a media or similar, and in most cases I end up having to see locally what it looks like.

@MrGVSV
Copy link
Member Author

MrGVSV commented Feb 13, 2024

I added the changes suggested by @BD103. Unfortunately, it seems adding HTML in the markdown is causing a CI failure?

@pablo-lua
Copy link
Contributor

Looks like some configuration is warning about no inline html in md files, which is bad in this case, as github only supports this collapsible thing with html 🤔

@alice-i-cecile
Copy link
Member

That should be able to be disabled in mdlint: locally would be ideal.

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

I like this change. I've used a showcase section in a few of my recent PRs, and I think it's really helped reviewers understand my changes.

@cart
Copy link
Member

cart commented Feb 22, 2024

As a (potential) counterargument: a Showcase section has a high risk of being redundant with the Solution section. I've tended to write "release note style showcases" of my features in the normal body of my pull request description:

#8624
#9885
#1525

@MrGVSV
Copy link
Member Author

MrGVSV commented Feb 22, 2024

As a (potential) counterargument: a Showcase section has a high risk of being redundant with the Solution section. I've tended to write "release note style showcases" of my features in the normal body of my pull request description:

#8624 #9885 #1525

I think this is a good point to call out. I'd argue, though, that creating this split allows the Solution section to be more "reviewer-facing" (e.g. why did we choose this approach, are there open questions or followup work, etc.). The Showcase section can then be "user-facing" and a lot more brief/showy.

Would it help if we intentionally call that out in the template?

@alice-i-cecile
Copy link
Member

Related discussion is occurring in the tools for migration guides thread on Discord. I've proposed a "S-Needs-Release-Note" label, which would then be read and used to populate stub sections of the upcoming release notes (and do the same thing for migration guides.

@janhohenheim
Copy link
Member

janhohenheim commented Jun 25, 2024

I think the workflow used for the 0.14 RC makes this PR obsolete, doesn't it @alice-i-cecile ?

@MrGVSV
Copy link
Member Author

MrGVSV commented Jun 25, 2024

I think the workflow used for the 0.14 RC makes this PR obsolete, doesn't it @alice-i-cecile ?

In what way? I think it helps prompt authors to help draft the release notes for their PR, but it's not always the PR author that does those (they may not want to, maybe no longer have the time, etc). I feel that a showcase section at least helps the other writers have a launching point (maybe not even needing to write anything extra), especially if they weren't originally involved with its development.

@alice-i-cecile
Copy link
Member

I think that the 0.14 workflow makes this more useful actually: if this gets merged we can prepopulate the release notes with this section if it exists.

@janhohenheim
Copy link
Member

Oh, fair enough, you're both right :)

@janhohenheim janhohenheim added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 26, 2024
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Jun 26, 2024
@alice-i-cecile
Copy link
Member

@MrGVSV can you cut the Changelog section from this PR? That one is definitely redundant, and I'd like to avoid lengthening the template further.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 1, 2024
@MrGVSV
Copy link
Member Author

MrGVSV commented Jul 5, 2024

Oops totally borked that rebase 😅

@MrGVSV MrGVSV added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 5, 2024
@alice-i-cecile
Copy link
Member

@MrGVSV mdlint is angry with you, but once you appease it I'll happily merge this in.

@janhohenheim
Copy link
Member

janhohenheim commented Jul 8, 2024

@alice-i-cecile imo mdlint is wrong here; a PR template should be allowed to use <details>.
For reference, this is how we allow specific HTML tags in markdown for the Rust game dev newsletter: https://github.com/rust-gamedev/rust-gamedev.github.io/blob/source/.markdownlint.json#L16-L20

@MrGVSV
Copy link
Member Author

MrGVSV commented Jul 8, 2024

Should I update the mdlint config?

@alice-i-cecile
Copy link
Member

Yes, please do :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 8, 2024
Merged via the queue into bevyengine:main with commit 2968d41 Jul 8, 2024
27 checks passed
@MrGVSV MrGVSV deleted the patch-3 branch July 8, 2024 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Meta About the project itself S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants