Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs
Thomas Rast writes: > Junio C Hamano writes: > >> Thomas Rast writes: >> >>> +test_extra_arg () { >>> + expect="success" >>> + if test "z$1" = "z-f"; then >>> + expect=failure >>> + shift >>> + fi >>> + test_expect_$expect "extra args: $*" " >>> + test_must_fail git remote $* bogus_extra_arg 2>actual && >>> + grep '^usage:' actual >>> + " >>> +} >>> + >>> +test_extra_arg -f add nick url >>> +test_extra_arg rename origin newname >> >> Perhaps just a taste in readability thing, but I would prefer to see >> them more like >> >> test_extra_arg_expect failure add nick url >> test_extra_arg_expect success rename origin newname >> >> than misunderstanding-inviting "-f" that often stands for "--force". > > Hmm. I had that at first, but then the final cleanup would have had to > touch all tests to remove the optional argument, making it noisy. You do not need a final cleanup, as I _never_ meant failure/success in the above illustration to be _optional_. Being explicit reduces mental burden when you later have to read such a custom scaffolding each test invents in an ad-hoc manner to suit its needs. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs
Junio C Hamano writes: > Thomas Rast writes: > >> +test_extra_arg () { >> +expect="success" >> +if test "z$1" = "z-f"; then >> +expect=failure >> +shift >> +fi >> +test_expect_$expect "extra args: $*" " >> +test_must_fail git remote $* bogus_extra_arg 2>actual && >> +grep '^usage:' actual >> +" >> +} >> + >> +test_extra_arg -f add nick url >> +test_extra_arg rename origin newname > > Perhaps just a taste in readability thing, but I would prefer to see > them more like > > test_extra_arg_expect failure add nick url > test_extra_arg_expect success rename origin newname > > than misunderstanding-inviting "-f" that often stands for "--force". Hmm. I had that at first, but then the final cleanup would have had to touch all tests to remove the optional argument, making it noisy. Anyway, it's probably all a bit over-engineered for only one effective code change ;-) -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs
Thomas Rast writes: > +test_extra_arg () { > + expect="success" > + if test "z$1" = "z-f"; then > + expect=failure > + shift > + fi > + test_expect_$expect "extra args: $*" " > + test_must_fail git remote $* bogus_extra_arg 2>actual && > + grep '^usage:' actual > + " > +} > + > +test_extra_arg -f add nick url > +test_extra_arg rename origin newname Perhaps just a taste in readability thing, but I would prefer to see them more like test_extra_arg_expect failure add nick url test_extra_arg_expect success rename origin newname than misunderstanding-inviting "-f" that often stands for "--force". Other than that, the whole series was a pleasant read. Thanks. > +test_extra_arg remove origin > +test_extra_arg set-head origin master > +# set-branches takes any number of args > +test_extra_arg set-url origin newurl oldurl > +test_extra_arg -f show origin > +test_extra_arg -f prune origin > +# update takes any number of args > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html