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

chore: more concise PR template #998

Merged
merged 1 commit into from
Jul 31, 2023
Merged

chore: more concise PR template #998

merged 1 commit into from
Jul 31, 2023

Conversation

moul
Copy link
Member

@moul moul commented Jul 29, 2023

  • skip one redundant title
  • make the top suggestion a comment instead of plain text
Contributor checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

- skips one redundant title
- make the top suggestion a comment instead of plain text
@moul moul requested a review from a team as a code owner July 29, 2023 12:39
@harry-hov
Copy link
Contributor

Can we do something about broken links in last point of the checklist?

[ ] Added new benchmarks to generated graphs, if any. More info here.

@moul
Copy link
Member Author

moul commented Jul 30, 2023

I've been thinking about removing that line since it's rarely addressed. My suggestion is to include a dedicated paragraph in the contribution guidelines and open an issue to discuss adding more useful benchmarks. What do you think?

@harry-hov
Copy link
Contributor

I think it's okay to remove it. The links are broken anyways. (we can always add things back if needed)

Sorry, I don't have much context about this point and I'm not sure what exactly contributors are supposed to do.
But, can we automate the benchmarking process instead of asking contributors to do it?

@ajnavarro ajnavarro mentioned this pull request Jul 31, 2023
7 tasks
@ajnavarro
Copy link
Contributor

My comments, opinions, and explanations of whys here:

  • 👍 to move the explanation about adding the description to a comment. That info is still visible to the PR author, and you do not need to delete that part of the template.
  • 👎 to keep checks inside a summary tag: why? Checks are there for a reason, and adding a way to ignore them is not the way to go IMHO. Checks are there to remember people's tasks that cannot be automated and must be done on every PR. These checks help the author not forget anything and help the reviewer to make a more straightforward due diligence process, based on trust in the author.
  • 👎 to remove benchmark check: I would love to be able to "automate" benchmarks. If someone has a good idea to do it, that would be awesome. The first link is broken because we were waiting until having a custom action runner to be able to run the tests (that is why the reports are not generated yet giving us a 404). That part is almost done by @albttx , so that link will eventually be working. The second link is a relative one, and that seems to not work on templates. Created a PR fixing it: chore(.github): fix broken link #1005. Even if right now is not a common thing to do, is something I would encourage (that check is encouraging it). Benchmarks are really important to validate ideas and new developments, and they will give us enormous insights into how code evolves through time. @harry-hov : here you have more context about that check: feat: add benchmarks and performance checks on PRs #881

@moul moul merged commit 6c9a1c6 into master Jul 31, 2023
6 of 7 checks passed
@moul moul deleted the moul-patch-3 branch July 31, 2023 09:14
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
@moul moul added this to the 💡Someday/Maybe milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants