timothygrant80 / cisTEM

Other
32 stars 26 forks source link

Suggested modification to PR template #421

Open bHimes opened 1 year ago

bHimes commented 1 year ago

I've found PRs are far more helpful in understanding changes when looking back than commit messages. Looking for any other feedback before updating our PR template to be more useful:

1- I think the section on which compilers were tested is redundant, given our CI setup, and could be removed.

2- Additionally, the question about how things were tested might be changed to get rid of the tests that are also in CI. Something like

If your changes aren't covered by unit, console, or samples tests, how did you test and confirm?

3 - Maybe split "Description" into "Main effect" and "Details (why)" "Details (what)" or something.

4 - We might also change the default behavior from "squash and merge" to "rebase and merge." The "git bisect" tool is super useful for going back and isolating regressions, but it is severely impeded when we squash several (or in the case of Tim, dozens 😉 ) of commits into a single one.

I think it just defaults to the last used, so it will already have changed based on my last merge, but I thought it would be good to point out this was intentional and see if you have any opinions on the topic.

jojoelfe commented 1 year ago

1- Agreed

2- Makes sense, too

3- Sure that sounds ok. I was thinking whether we should require a one-liner that later could go into a changelog. Something like: [Bug fix] Image class now uses correct dimensions or [Feature] apply_ctf now allows for application of a wiener filter or [Performance] match_template is now 10x faster

4 - This might depend a lot on the size of the PR and the style of the author. I worry that for a lot of my PRs it would include a lot of commits that say "Argh, I'm sooo stupid"

bHimes commented 1 year ago

I like the changelog idea!

To point 4) you could always do an interactive rebase and force push to clean up, but tbh, I never look at commit messages except for live branches that I haven't worked on for a week or so.

Where I have spent a lot of time is using git bisect to locate a "first broken commit" only to have that commit have so many changes that I still spend day(s) debugging.

Having a bunch of "oops I fucked up" commits adds a little noise to a search with Git bisect, especially if they don't compile (git bisect skip is fine for that situation) but on the whole I would venture to say this might add one or two iterations of bisect, which is effectively nothing.

I would much rather have a finer commit granularity with a little noise I can filter out.

Git bisect aside, Git log has a ton regex capability so it would be trivial to filter out "WIP" and "fuuhhhhck me" etc : )

bHimes commented 1 year ago

@jojoelfe what do you think about this option regarding your suggestion for number 3?