bevy icon indicating copy to clipboard operation
bevy copied to clipboard

meta: Add `Showcase` section to PR template

Open MrGVSV opened this issue 2 years ago • 13 comments

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 avatar Feb 07 '24 02:02 MrGVSV

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.");

BD103 avatar Feb 09 '24 18:02 BD103

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

MrGVSV avatar Feb 13 '24 01:02 MrGVSV

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 🤔

pablo-lua avatar Feb 13 '24 01:02 pablo-lua

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

alice-i-cecile avatar Feb 13 '24 01:02 alice-i-cecile

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:

https://github.com/bevyengine/bevy/pull/8624 https://github.com/bevyengine/bevy/pull/9885 https://github.com/bevyengine/bevy/pull/1525

cart avatar Feb 22 '24 21:02 cart

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?

MrGVSV avatar Feb 22 '24 21:02 MrGVSV

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.

alice-i-cecile avatar Feb 22 '24 21:02 alice-i-cecile

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

janhohenheim avatar Jun 25 '24 20:06 janhohenheim

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.

MrGVSV avatar Jun 25 '24 22:06 MrGVSV

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.

alice-i-cecile avatar Jun 25 '24 22:06 alice-i-cecile

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

janhohenheim avatar Jun 26 '24 08:06 janhohenheim

@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 avatar Jul 01 '24 13:07 alice-i-cecile

Oops totally borked that rebase 😅

MrGVSV avatar Jul 05 '24 00:07 MrGVSV

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

alice-i-cecile avatar Jul 08 '24 00:07 alice-i-cecile

@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

janhohenheim avatar Jul 08 '24 00:07 janhohenheim

Should I update the mdlint config?

MrGVSV avatar Jul 08 '24 00:07 MrGVSV

Yes, please do :)

alice-i-cecile avatar Jul 08 '24 00:07 alice-i-cecile