FuelLabs / fuels-ts

Fuel Network Typescript SDK
https://docs.fuel.network/docs/fuels-ts/
Apache License 2.0
44.4k stars 1.32k forks source link

Consider adding a PR template #2649

Open arboleya opened 3 days ago

arboleya commented 3 days ago

By standardizing our PR templates, we can automate other routines around it.

One idea is to have all breaking changes consolidated in the release notes.

This is possible by slicing the PR description following a pre-defined template.

Check two ideas in the comments below:

  1. Full
  2. Minimal
arboleya commented 3 days ago

Summary

Breaking Changes

The return of the following methods was modified to support pagination:

// before
abc
// before
xyz

Checklist

arboleya commented 3 days ago

Checklist

arboleya commented 3 days ago

We should also consider a CI check to validate that breaking !PRs have detailed breaking notes.

arboleya commented 3 days ago

Real-world example here.

maschad commented 3 days ago

Thanks for opening this @arboleya this will definitely help in the review process.

My preference is for the full template

I think we should ensure we include comments in the PR template to indicate what the sections should be populated with, so for example

<!--
Please write a summary of your changes and why you made them.
Please format the issue you are closing with a - beside it, for example:
- Closes https://github.com/FuelLabs/fuels-ts/issues/ABCD.
-->

# Summary

Also I think we should be a bit more explicit in the Checklist so that external contributors have no ambiguity about what is required before opening a PR for review.

My suggestion would be:

Checklist

We could also highlight in the CONTRIBUTING.md that PRs should be in draft until they have completed the checklist to make reviews more seamless.

Otherwise everything else looks good.

arboleya commented 2 days ago

The minimal template is the same as the full one but without "Summary" or "Breaking Changes."

Not all PRs will be complex or substantial enough to require all these sections; some can be optional.

As you exemplified, I agree with replacing the placeholder/demo texts with comments.

I was hesitant about having checklists because they become white noise.

We can try a few options to see if there's a nice middle ground.

Here's another:

The latter is questionable, but it can have its utility.

PR assignment, label, and milestone can be left out.

maschad commented 2 days ago

The minimal template is the same as the full one but without "Summary" or "Breaking Changes."

Not all PRs will be complex or substantial enough to require all these sections; some can be optional.

As you exemplified, I agree with replacing the placeholder/demo texts with comments.

I was hesitant about having checklists because they become white noise.

Thanks for the clarification, in that case I agree.

We can try a few options to see if there's a nice middle ground.

Here's another:

  • [ ] I addedtests to prove my changes
  • [ ] I updated — all the necessary documentation
  • [ ] I reviewed — the entire PR myself, using the GitHub UI
  • [ ] I described — all Breaking Changes and Migration Guide
  • [ ] I checked — the impact of Breaking Changes on existing apps

The latter is questionable, but it can have its utility.

PR assignment, label, and milestone can be left out.

That's reasonable, I only suggested the milestone and assignee because it's apart of our review process, but I don't have a strong opinion on it, I think the above checklist is sufficient as well.

danielbate commented 1 day ago

I also prefer the full template.

I'm also not a huge fan of checkboxes but let's see, these ones seem reasonable.

I checked — the impact of Breaking Changes on existing apps

What are we expecting for this? Existing apps being those outside the SDK? As we removed the breaking change workflow in #2631 and I can't see externals doing this. Internal app breakages should be immediately apparent with a failed build.

petertonysmith94 commented 1 day ago

The full template would also be my preferred.

I'm also not a huge fan of checkboxes but let's see, these ones seem reasonable.

I also second this, but I'm open to trialing it out.