On 2024-03-18 20:17, Eric Sunshine wrote:
On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@xxxxxxxxxxx>
wrote:
On 2024-03-18 03:48, Eric Sunshine wrote:
> Readability wasn't my reason for bringing this up. As a reviewer,
> every time a question pops into my mind as I'm reading the code, that
> indicates that something about the code is unclear or that the commit
> message doesn't properly explain why it was done in this way. People
> coming across this code in the future may have the same questions but
> they won't have the benefit of being able to easily ask you why it was
> done this way.
I see. How about including a small comment in the t1300 that would
explain the additional indentation?
I'm just one reviewer. Unless others chime in with similar
observations or questions regarding the patch, I don't think such a
comment is necessary. Aside from the other more significant points
(such as not introducing x_to_tab(), using "setup" in the function
title, etc.), this is extremely minor, and what you have here is "good
enough" (though you may want to take Junio's suggestion of using a
leading "|" to protect indentation).
Just to reiterate, both x_to_tab() and the test naming have already
been addressed in the future v3 of this series.
As a note, there are already more tests in the t1300 that contain such
indentation, so maybe we shoulddo something with those existing tests
as well; the above-proposed comment, which would be placed at the
very
beginning of t1300, may provide a satisfactory explanation for all the
tests in t1300 that contain such additional indentation.
Another option would be to either add the indentation to all relevant
tests in the t1300, or to remove the indentation from all tests in the
t1300 that already contain it. I'd be happy to implement and submit
patches that do that, after we choose the direction we want to follow.
It would be better to keep this series focused on its primary goal of
fixing a bug rather than being held hostage to an ever increasing set
of potential cleanups. Such cleanups can be done as separate patch
series either atop this series or alongside it. Let's land this series
first, and then, if you wish, tackle those other less significant
issues.
Thanks, I totally agree.
> If these new tests are also checking leading whitespace behavior, then
> to improve coverage, would it make sense to have the leading "X" on
> some lines but not others?
Good point, despite that not being the main purpose of the added
tests.
I'll see to add a couple of tests that check the handling of
indentation,
possibly at some places in the t1300 that fit the best; improving the
tests coverage can only help in the long run.
As above, such additional tests probably aren't mandatory for this
bug-fix series. As a reviewer, I'd like to see fewer and fewer changes
between each version of a patch series; the series should converge so
that it can land rather than diverge from iteration to iteration. Such
additional leading-whitespace tests may be perfectly appropriate for a
follow-up series.
Agreed once again. Let's wrap this up, and I'll come back with the
follow-up patches.
