| Message ID | 20201110212136.870769-7-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: bash: a bunch of fixes, cleanups, and reorganization | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > Pretty straightforward: runs functions. Hmph, sorry but this is not straight-forward at least to me. Yes, the helper runs whatever is given on the command line, but then it does "print_comp", too. And the proposed log message is not entirely clear on the most important thing: why? What is this "helper" meant to help? Reduce repetition? > +run_func () > +{ > + local -a COMPREPLY && > + "$@" && print_comp > +} > + > # Test high-level completion > # Arguments are: > # 1: typed text so far (cur) > @@ -452,8 +458,7 @@ test_expect_success '__gitcomp_direct - puts everything into COMPREPLY as-is' ' > EOF > ( > cur=should_be_ignored && > - __gitcomp_direct "$(cat expected)" && > - print_comp > + run_func __gitcomp_direct "$(cat expected)" This is an no-op rewrite, as we used to do the gitcomp-direct followed by print-comp, which is exactly what the helper does. So the helper does reduce repetition, which by itself would be a good thing but is there other benefit we are trying to seek (there does not have to be any)? > test_expect_success '__gitcomp - doesnt fail because of invalid variable name' ' > - __gitcomp "$invalid_variable_name" > + run_func __gitcomp "$invalid_variable_name" This one changes the behaviour in that it starts running print_comp which we didn't run. It may be a good thing and improvement, but then we'd better advertise it in the proposed log message. > ' > > read -r -d "" refs <<-\EOF > @@ -586,7 +591,7 @@ test_expect_success '__gitcomp_nl - no suffix' ' > ' > > test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' ' > - __gitcomp_nl "$invalid_variable_name" > + run_func __gitcomp_nl "$invalid_variable_name" Likewise. > ' Thanks.
On Wed, Nov 11, 2020 at 1:27 AM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Pretty straightforward: runs functions. > > Hmph, sorry but this is not straight-forward at least to me. Yes, > the helper runs whatever is given on the command line, but then it > does "print_comp", too. And the proposed log message is not > entirely clear on the most important thing: why? > > What is this "helper" meant to help? Reduce repetition? Well, I thought the "helper" part of the title made it obvious: the helper function does help in not having to type the same code over and over. But there's in fact multiple benefits. 1. It makes the code more consistent. Everything in the test script either calls a test_ function, or a run_ function, except the code that is testing the functions directly. 2. It reduces code (obvious in a helper function), as the same __gitcomp* && print_comp is executed over and over, with zero variation. 3. It makes the code more maintainable (I also thought this was obvious); if we want to add something we don't have to do it dozens of times, we just do it on the helper function. Is this enough of a "why"? It is in fact number 3 the one I'm after, and a line that shouldn't be part of this patch was smuggled in, so perhaps that's why future patches don't obviate the need for this one. But even with no other reason for it, the patch stands on its own. > > +run_func () > > +{ > > + local -a COMPREPLY && This is the line that was smuggled in. It should be part of a separate patch, since this is behavior change. > > + "$@" && print_comp > > +} > > + > > # Test high-level completion > > # Arguments are: > > # 1: typed text so far (cur) > > @@ -452,8 +458,7 @@ test_expect_success '__gitcomp_direct - puts everything into COMPREPLY as-is' ' > > EOF > > ( > > cur=should_be_ignored && > > - __gitcomp_direct "$(cat expected)" && > > - print_comp > > + run_func __gitcomp_direct "$(cat expected)" > > This is an no-op rewrite, as we used to do the gitcomp-direct > followed by print-comp, which is exactly what the helper does. So > the helper does reduce repetition, which by itself would be a good > thing but is there other benefit we are trying to seek (there does > not have to be any)? It's not exactly a no-op, since I cleared COMPREPLY. That should be done in a separate patch so it's truly a no-op. It is the clearing of COMPREPLY that I'm eventually interested in. First; that's how the testing framework is supposed to work: test #1 should not interfere with test #2, but second; once the gitcomp functions are changed to append code instead of clearing COMPREPLY by themselves and then appending, this prevents the tests from failing. > > test_expect_success '__gitcomp - doesnt fail because of invalid variable name' ' > > - __gitcomp "$invalid_variable_name" > > + run_func __gitcomp "$invalid_variable_name" > > This one changes the behaviour in that it starts running print_comp > which we didn't run. It may be a good thing and improvement, but > then we'd better advertise it in the proposed log message. But nothing is done with the output; the behavior doesn't change. The test still passes or fails irrespective of what print_comp does. test_expect_success 'test 1' 'true' test_expect_success 'test 2' ': echo foobar' test_expect_success 'test 3' 'echo foobar > /dev/null' These three tests may do different things, but their behavior is the same, that is to say: with the same input they generate the same output. Do you want me to add: "In two places we generate an output that didn't exist before, but nothing ever reads it." ? Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > But even with no other reason for it, the patch stands on its own. > >> > +run_func () >> > +{ >> > + local -a COMPREPLY && > > This is the line that was smuggled in. It should be part of a separate > patch, since this is behavior change. > ... > Do you want me to add: "In two places we generate an output that > didn't exist before, but nothing ever reads it." ? That would be very friendly to readers who may later wonder why the change was made, yes. In any case, I am not a shell-completion person, so even if I said "yes that would make the patch perfect", that would not count as much ;-)
On Wed, Nov 11, 2020 at 10:39 AM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > But even with no other reason for it, the patch stands on its own. > > > >> > +run_func () > >> > +{ > >> > + local -a COMPREPLY && > > > > This is the line that was smuggled in. It should be part of a separate > > patch, since this is behavior change. > > ... > > Do you want me to add: "In two places we generate an output that > > didn't exist before, but nothing ever reads it." ? > > That would be very friendly to readers who may later wonder why the > change was made, yes. > > In any case, I am not a shell-completion person, so even if I said > "yes that would make the patch perfect", that would not count as > much ;-) But it doesn't hurt either. I'll resend the series only with the obvious fixes, since there doesn't seem to be much interest in the rest.
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 2e04462bb0..3ec9ff9308 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -75,6 +75,12 @@ run_completion () __git_wrap__git_main && print_comp } +run_func () +{ + local -a COMPREPLY && + "$@" && print_comp +} + # Test high-level completion # Arguments are: # 1: typed text so far (cur) @@ -452,8 +458,7 @@ test_expect_success '__gitcomp_direct - puts everything into COMPREPLY as-is' ' EOF ( cur=should_be_ignored && - __gitcomp_direct "$(cat expected)" && - print_comp + run_func __gitcomp_direct "$(cat expected)" ) && test_cmp expected out ' @@ -547,7 +552,7 @@ test_expect_success '__gitcomp - equal skip' ' ' test_expect_success '__gitcomp - doesnt fail because of invalid variable name' ' - __gitcomp "$invalid_variable_name" + run_func __gitcomp "$invalid_variable_name" ' read -r -d "" refs <<-\EOF @@ -586,7 +591,7 @@ test_expect_success '__gitcomp_nl - no suffix' ' ' test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name' ' - __gitcomp_nl "$invalid_variable_name" + run_func __gitcomp_nl "$invalid_variable_name" ' test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from config file' ' @@ -1086,8 +1091,7 @@ test_expect_success '__git_complete_refs - simple' ' EOF ( cur= && - __git_complete_refs && - print_comp + run_func __git_complete_refs ) && test_cmp expected out ' @@ -1099,8 +1103,7 @@ test_expect_success '__git_complete_refs - matching' ' EOF ( cur=mat && - __git_complete_refs && - print_comp + run_func __git_complete_refs ) && test_cmp expected out ' @@ -1113,8 +1116,7 @@ test_expect_success '__git_complete_refs - remote' ' EOF ( cur= && - __git_complete_refs --remote=other && - print_comp + run_func __git_complete_refs --remote=other ) && test_cmp expected out ' @@ -1132,8 +1134,7 @@ test_expect_success '__git_complete_refs - track' ' EOF ( cur= && - __git_complete_refs --track && - print_comp + run_func __git_complete_refs --track ) && test_cmp expected out ' @@ -1145,8 +1146,7 @@ test_expect_success '__git_complete_refs - current word' ' EOF ( cur="--option=mat" && - __git_complete_refs --cur="${cur#*=}" && - print_comp + run_func __git_complete_refs --cur="${cur#*=}" ) && test_cmp expected out ' @@ -1158,8 +1158,7 @@ test_expect_success '__git_complete_refs - prefix' ' EOF ( cur=v1.0..mat && - __git_complete_refs --pfx=v1.0.. --cur=mat && - print_comp + run_func __git_complete_refs --pfx=v1.0.. --cur=mat ) && test_cmp expected out ' @@ -1175,8 +1174,7 @@ test_expect_success '__git_complete_refs - suffix' ' EOF ( cur= && - __git_complete_refs --sfx=. && - print_comp + run_func __git_complete_refs --sfx=. ) && test_cmp expected out ' @@ -1189,8 +1187,7 @@ test_expect_success '__git_complete_fetch_refspecs - simple' ' EOF ( cur= && - __git_complete_fetch_refspecs other && - print_comp + run_func __git_complete_fetch_refspecs other ) && test_cmp expected out ' @@ -1201,8 +1198,7 @@ test_expect_success '__git_complete_fetch_refspecs - matching' ' EOF ( cur=br && - __git_complete_fetch_refspecs other "" br && - print_comp + run_func __git_complete_fetch_refspecs other "" br ) && test_cmp expected out ' @@ -1215,8 +1211,7 @@ test_expect_success '__git_complete_fetch_refspecs - prefix' ' EOF ( cur="+" && - __git_complete_fetch_refspecs other "+" "" && - print_comp + run_func __git_complete_fetch_refspecs other "+" "" ) && test_cmp expected out ' @@ -1229,8 +1224,7 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified' ' EOF ( cur=refs/ && - __git_complete_fetch_refspecs other "" refs/ && - print_comp + run_func __git_complete_fetch_refspecs other "" refs/ ) && test_cmp expected out ' @@ -1243,8 +1237,7 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' ' EOF ( cur=+refs/ && - __git_complete_fetch_refspecs other + refs/ && - print_comp + run_func __git_complete_fetch_refspecs other + refs/ ) && test_cmp expected out ' @@ -1775,8 +1768,7 @@ test_path_completion () # unusual characters in path names. By requesting only # untracked files we do not have to bother adding any # paths to the index in those tests. - __git_complete_index_file --others && - print_comp + run_func __git_complete_index_file --others ) && test_cmp expected out } @@ -2261,8 +2253,7 @@ do ( words=(git push '$flag' other ma) && cword=${#words[@]} cur=${words[cword-1]} && - __git_complete_remote_or_refspec && - print_comp + run_func __git_complete_remote_or_refspec ) && test_cmp expected out ' @@ -2274,8 +2265,7 @@ do ( words=(git push other '$flag' ma) && cword=${#words[@]} cur=${words[cword-1]} && - __git_complete_remote_or_refspec && - print_comp + run_func __git_complete_remote_or_refspec ) && test_cmp expected out '
Pretty straightforward: runs functions. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- t/t9902-completion.sh | 58 ++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 34 deletions(-)