Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success
- From
Shuqi Liang <cheskaqiqi@gmail.com>
- Date
- Feb 15, 2023, 02:40 UTC
- Message-ID
- <CAMO4yUHx68jSe31gKon6_hXTiqufHnHAFkP32DXmWg7dtagAfQ@mail.gmail.com>
- In-Reply-To
- <CAPig+cTKCMwCQ2TxXyc1KquZmB0QWQAuuag52a1qmeYh6XPXYQ@mail.gmail.com>
On Tue, Feb 14, 2023 at 8:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
> Although it's true that t/README explains why code should be placed > inside tests, you can help readers out by simply explaining the reason > here in the commit message. For instance, you might replace the above > paragraph with: > > Some old test scripts have setup code outside of tests. This > is problematic since any failures of the setup code will go > unnoticed. Therefore, move setup code into the tests themselves > so that failures are properly flagged. >
Thanks, that really makes the change more clear for the readers. I learn a lot.
Show 10 quoted lines
> Reviewers appreciate well-explained commit messages, but they also > appreciate succinctness. Although it may not always be obvious how > much to write in a commit message, you can assume that reviewers will > understand obvious changes simply by reading the patch itself, thus > you don't need to mention every little detail in the commit message. > The important thing to mention in the commit message is the > explanation of _why_ the change is being made, plus any changes which > might not be obvious. In this case, all the changes are obvious, so, > really, you can collapse this entire commit message to just the first > paragraph.
Yeah, the commit looks very wordy. I'll make it more succinct.
> test_write_lines a b c >file &&
Show 5 quoted lines
> Same comment about simply using double quotes instead of > single-quotes, however, this is also another really good place to use > test_write_lines: > > test_write_lines a b c >file &&
Thanks for the tips!
(Sorry for sending the V6 to you twice I send it by accident .)
------------------------- Thanks Shuqi