Closed kenbowen closed 4 years ago
I added a title and description to try and explain why the proposed changes are needed. Please check them over and edit as necessary -- I had to make educated guesses, because none of the commit messages say why the changes was made.
The PR title/description will be used as the merge commit comment, so that will explain the reasoning to our future selves.
Your educated guesses were right on the button. I didn’t insert any significant explanations because I’ve got an “of course, we need to doc the builtins & library, and doctools was built for that" assumption in my head. But you’re right: it should be explained. Who knows, with my sieve of a memory, maybe I’ll forget why someday :-).
On Apr 10, 2020, at 9:01 AM, Chuck Houpt notifications@github.com wrote:
@chuckhoupt approved this pull request.
Looks fine, as long as final merge or squash-n-merge commit explains why the changes were made (see proposed title/description I added to PR).
If you wanted to make it absolutely perfect, you could interactively-rebase the branch to reduce to 3 commits to match the 3 "whys" in the PR description, and then a rebase-n-merge the PR. But, that would be a lot of work and "perfect is the enemy of good-enough".
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.
The PR title/description will be used as the merge commit comment, so that will explain the reasoning to our future selves.
Oops, looks like I was wrong about how Github constructs merge commit messages -- the message is just a generic "merged PR #XXX". Next time, we'll have to do a squash or something.