issue #129 - Adds new -m option for adding commit message body to generated fixups #176
Conversation
arielf212
force-pushed
the
issue-129
branch
2 times, most recently
from
97cbf6d to
8b8c347
Compare
|
I think I'm doing a lot of stupid mistakes since its already night were I'm at. Thanks for the helpful review so far! @blairconrad |
This option is used to specify the commit body for generated fixup commits
There was a problem hiding this comment.
Thanks for including me, @arielf212. I'm enjoying the changes. I made some nitpicky comments about intention-revealing test names, as is my wont.
src/lib.rs
Outdated
| @@ -978,6 +985,59 @@ mod tests { | |||
| assert_eq!(actual_msg, expected_msg); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn fixup_message_config_not_provided_doesnt_change_anything() { | |||
There was a problem hiding this comment.
nitpick: "doesnt change anything" may suggest to the reader that nothing is changed, but we expect git-absorb to make some new commits, etc.
fixup_message_config_provided_sets_message downstairs explicitly references the fact that it's the commit message that's affected. Consider renaming this test to indicate that it's really looking at the commit message.
another nitpick: for both this test and fixup_message_config_provided_sets_message, consider replacing "config" with "option". The other tests use "config" to mean that whatever is being tested is read from the git config. Otherwise they use "flag" or "option" as appropriate.
There was a problem hiding this comment.
Its a convention I carry from other codebases I worked as, wherein (simple) unit tests usually describe a flow option and then what it does to the state of the program in relation to what was before its existence.
I'll change it to your convention, since I really don't care about test names that much.
There was a problem hiding this comment.
This is interesting to me. If I understand correctly, it's about the current change?
In the context of a PR, I can see that being useful, but I wonder how it stands up 8 years later when a new person reads the test. How do they know the context? Are they expected to find the commit that added the test, diff it (or look at the PR) and then proceed?
There was a problem hiding this comment.
humans do not generally read test code for fun :)
Some test are there to show examples of how to use a library/component, but I think those fit in another category and I'll generally write a lot of docstrings above tests I intend to have for this purpose.
In general, if you are reading a test, its probably because something broke. In that case, I think that the correct flow is to go read the PR that introduced the test and see why it was added.
Also I believe that test code should be self-explanatory, which is not currently happening in this repo, but thats another discussion altogether.
This option is used to specify the commit body for generated fixup commits.
Closes #129 .