-
Notifications
You must be signed in to change notification settings - Fork 185
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: Remove checkboxes from PR templates #3124
base: main
Are you sure you want to change the base?
Conversation
A significant number of our own PRs don't bother with the checkboxes. This makes it hard to enforce (I've tried!) and does not give us anything to ask from external contributors either. I'm also often torn what to do with non-applicable items, I've tried all of: - Remove them. Weird. - Strike them out, not so weird, but so much effort. - Tick off even if not-applicable (my current approache), also weird. So instead of bothering the humans I'm adapting to their behaviour. An alternative I could live with would be enforce this by machines: Have a required CI check that needs the checkboxes ticked off so that you can not merge a PR while ignoring this. Maybe with some more guidance on what to do if not applicable.
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3124/docs/iroh/ Last updated: 2025-01-13T12:01:02Z |
- Self-review. | ||
- Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text). | ||
- Tests. | ||
- Document breaking changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe put them behind html comments, so they don't show up in the actual issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that desired though? Then folks will just not see this at all.
I get that it could be nice to hide them in the commit messages, but the html comments do not achieve that I think.
FWIW, I at least like the self-review checkbox, and regularly leave that unchecked in draft PRs so I remember to go through PRs once more when I undraft. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, to be honest, I really like having these as checkboxes for myself.
If one doesn't fit, I just delete the line & I think it's fine for a developer to decide that one of the todos isn't relevant for their PR.
I also don't mind having these in each merge commit, since it gives a general overview of "hey no tests were added or fixed" or "no breaking changes in this commit"
So FWIW my personal opinion is that I like these. But I review so many PRs where they are ignored and that inconsistency is what bothers me as I never know how to handle it. So this PR is my attempt at accepting the status quo. I'd be fine with requiring these checkboxes, but if that's the case it should probably be done by automation rather than by reviewers. So I'd be totally on board with the alternative of enforcing them in CI like we enforce the PR title already. Or maybe the checkboxes are just optional to use in case the author likes to use them? FWIW, I asked a review from everyone individually because I do think something like this needs broad acceptance. I don't want to force this change at all. |
Perhaps being more clear about "the rules" here might be helpful.
(Perhaps in less words) And then in terms of rules:
This might or might not help. Unsure. Might also just be too much work. |
I could totally live with this text.
2 should certainly allowed. 1 is kind of optional with the suggested text, but we could do if it's simple to do. |
Description
A significant number of our own PRs don't bother with the checkboxes.
This makes the checkboxes themselves have low value, the number of
completed tasks in the PR list has no bearing on the state of PRs.
It also makes it hard to enforce (I've tried!) and does not give us
anything to ask from external contributors either.
I'm also often torn what to do with non-applicable items, I've tried
all of:
So instead of bothering the humans I'm adapting to their behaviour.
Breaking Changes
Notes & open questions
An alternative I could live with would be enforce this by machines:
Have a required CI check that needs the checkboxes ticked off so that
you can not merge a PR while ignoring this. Maybe with some more
guidance on what to do if not applicable.
Change checklist