[v2,2/4] t7401: change test_i18ncmp syntax for clarity
diff mbox series

Message ID 20200812192737.13971-3-shouryashukla.oo@gmail.com
State New
Headers show
Series
  • t7401: modernize, cleanup and more
Related show

Commit Message

Shourya Shukla Aug. 12, 2020, 7:27 p.m. UTC
Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to
'test_i18ncmp expected actual' to align it with the convention followed
by other tests in the test script.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t7401-submodule-summary.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Aug. 12, 2020, 8:57 p.m. UTC | #1
Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to
> 'test_i18ncmp expected actual' to align it with the convention followed
> by other tests in the test script.
> ...
> @@ -285,7 +285,7 @@ EOF
>  
>  test_expect_success '--for-status' "
>  	git submodule summary --for-status HEAD^ >actual &&
> -	test_i18ncmp actual - <<EOF
> +	test_i18ncmp - actual <<-EOF
>  * sm1 $head6...0000000:
>  
>  * sm2 0000000...$head7 (2):

This one does more than what the proposed log message explains, but
it does not do enough at the same time.  

If "actual vs expected order" is what this commit wants to fix, then
"<<EOF" vs "<<-EOF" is outside the scope of it.

Personally, I think it is a good idea to update the end-of-heredoc
marker to "<<-EOF", but the patch makes its readers wonder if the
author of the patch understands why it is a good idea to begin with,
because the end-of-heredoc marker is the only thing the patch
changes, and it does not change the indentation of heredoc itself.

The whole point of using "<<-EOF" instead of "<<EOF" is so that the
result becomes easier to read with indentation, e.g.

	test_i18ncmp - actual <<-EOF
	* sm1 $head6...0000000:

	* sm2 0000000...$head7 (2):
	...
	EOF

Compared to the original:

	test_i18ncmp - actual <<EOF
* sm1 $head6...0000000:

* sm2 0000000...$head7 (2):
...
EOF

it would be easier to read.
Junio C Hamano Aug. 12, 2020, 9:02 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>
>> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to
>> 'test_i18ncmp expected actual' to align it with the convention followed
>> by other tests in the test script.
>> ...
>> @@ -285,7 +285,7 @@ EOF
>>  
>>  test_expect_success '--for-status' "
>>  	git submodule summary --for-status HEAD^ >actual &&
>> -	test_i18ncmp actual - <<EOF
>> +	test_i18ncmp - actual <<-EOF
>>  * sm1 $head6...0000000:
>>  
>>  * sm2 0000000...$head7 (2):
>
> This one does more than what the proposed log message explains, but
> it does not do enough at the same time.  
>
> If "actual vs expected order" is what this commit wants to fix, then
> "<<EOF" vs "<<-EOF" is outside the scope of it.
>
> Personally, I think it is a good idea to update the end-of-heredoc
> marker to "<<-EOF", ...

It seems that the theme of your [3/4] is exactly about fixing the
"end-of-heredoc marker is prefixed with dash, but the heredoc is not
indented for readability", so perhaps you'd want to stop this step
at turning the line to

>> -	test_i18ncmp actual - <<EOF
>> +	test_i18ncmp - actual <<EOF

i.e. "compare expected vs actual in this order", and then in the
next patch [3/4], edit the line further to

	test_i18ncmp - actual <<-EOF

*and* indent the here-doc at the same time?
Shourya Shukla Aug. 14, 2020, 2:57 p.m. UTC | #3
On 12/08 02:02, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> >
> >> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to
> >> 'test_i18ncmp expected actual' to align it with the convention followed
> >> by other tests in the test script.
> >> ...
> >> @@ -285,7 +285,7 @@ EOF
> >>  
> >>  test_expect_success '--for-status' "
> >>  	git submodule summary --for-status HEAD^ >actual &&
> >> -	test_i18ncmp actual - <<EOF
> >> +	test_i18ncmp - actual <<-EOF
> >>  * sm1 $head6...0000000:
> >>  
> >>  * sm2 0000000...$head7 (2):
> >
> > This one does more than what the proposed log message explains, but
> > it does not do enough at the same time.  
> >
> > If "actual vs expected order" is what this commit wants to fix, then
> > "<<EOF" vs "<<-EOF" is outside the scope of it.
> >
> > Personally, I think it is a good idea to update the end-of-heredoc
> > marker to "<<-EOF", ...
> 
> It seems that the theme of your [3/4] is exactly about fixing the
> "end-of-heredoc marker is prefixed with dash, but the heredoc is not
> indented for readability", so perhaps you'd want to stop this step
> at turning the line to
> 
> >> -	test_i18ncmp actual - <<EOF
> >> +	test_i18ncmp - actual <<EOF
> 
> i.e. "compare expected vs actual in this order", and then in the
> next patch [3/4], edit the line further to
> 
> 	test_i18ncmp - actual <<-EOF
> 
> *and* indent the here-doc at the same time?

Ohh okay okay. I understand what you are saying. I wanted to fix the
heredoc markers and indent the tests for better readibility but actually
I fixed the heredoc marker in [2/4]. Therefore, the change in [2/4]
should in fact be:

    -	test_i18ncmp actual - <<EOF
    +	test_i18ncmp - actual <<EOF

And in this patch [3/4], it should become:

    -	test_i18ncmp - actual <<EOF
    +	test_i18ncmp - actual <<-EOF

As well as the indentation fixes I did in the patch already. Now I
understand the exact use and significance of the heredoc. Thank you.

Patch
diff mbox series

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 8ee78bcb69..53943c9cc9 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -183,7 +183,7 @@  test_expect_success 'typechanged submodule(submodule->blob), --cached' "
   < Add foo5
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 test_expect_success 'typechanged submodule(submodule->blob), --files' "
@@ -193,7 +193,7 @@  test_expect_success 'typechanged submodule(submodule->blob), --files' "
   > Add foo5
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 rm -rf sm1 &&
@@ -204,7 +204,7 @@  test_expect_success 'typechanged submodule(submodule->blob)' "
 * sm1 $head4(submodule)->$head5(blob):
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 rm -f sm1 &&
@@ -217,7 +217,7 @@  test_expect_success 'nonexistent commit' "
   Warn: sm1 doesn't contain commit $head4_full
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 commit_file
@@ -285,7 +285,7 @@  EOF
 
 test_expect_success '--for-status' "
 	git submodule summary --for-status HEAD^ >actual &&
-	test_i18ncmp actual - <<EOF
+	test_i18ncmp - actual <<-EOF
 * sm1 $head6...0000000:
 
 * sm2 0000000...$head7 (2):