Re: [PATCH v9 3/5] t4205, t6006, t7102: make functions more readable
Alexey Shumkin writes: >> > Could you point me to the coding style guide, please? >> >> Documentation/CodingGuidelines:: > Oh! :) > thank you The most important part of the coding guidelines is to match the style to existing code when writing a new one: $ git grep '^[a-z_]* ()' -- *.sh | wc -l 132 $ git grep '^[a-z_]*()' -- *.sh | wc -l 55 We have acquired quite a few violators, but that is no reason to add more. -- 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 v9 3/5] t4205, t6006, t7102: make functions more readable
On Fri, Jul 05, 2013 at 01:44:07AM -0700, Junio C Hamano wrote:
> Alexey Shumkin writes:
>
> >> Perhaps like this.
> >>
> >> Function 'test_format' has become harder to read after its
> >> change in de6029a2 (pretty: Add failing tests: --format output
> >> should honor logOutputEncoding, 2013-06-26). Simplify it by
> >> moving its "should we expect it to fail?" parameter to the end.
> > I'm not sure whether this "last parameter" is needed in that code as far as
> > we
> > already removed expected to fail tests
>
> Whatever.
>
> The above is an example of justifying a more vague "simple" ("is
> better" is implied) with a concrete point (i.e. By moving that to
> the end, you removed the need to conditionally shift $@ in the
> function to simplify the codepath), based on my _guess_ of what you
> possibly meant to say, from reading your description that did not
> give much clue for me to guess why you thought the result was "more
> elegant". If my guess missed what your true justification was,
> please replace it with the more correct one ;-)
Ok
>
> >> I cannot read why you think the updated commit_msg is "more pretty"
> >> in the message or in the patch.
> >>
> >> > -commit_msg () {
> >> > -# String "initial. initial" partly in German (translated with
> >> > Google Translate),
> >> > +commit_msg() {
> >>
> >> Style. Have SP on both sides of () in a shell function definition.
> > Could you point me to the coding style guide, please?
>
> Documentation/CodingGuidelines::
Oh! :)
thank you
>
> - We prefer a space between the function name and the parentheses. The
>opening "{" should also be on the same line.
>E.g.: my_function () {
Aha
--
Alexey Shumkin
--
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 v9 3/5] t4205, t6006, t7102: make functions more readable
Alexey Shumkin writes:
>> Perhaps like this.
>>
>> Function 'test_format' has become harder to read after its
>> change in de6029a2 (pretty: Add failing tests: --format output
>> should honor logOutputEncoding, 2013-06-26). Simplify it by
>> moving its "should we expect it to fail?" parameter to the end.
> I'm not sure whether this "last parameter" is needed in that code as far as we
> already removed expected to fail tests
Whatever.
The above is an example of justifying a more vague "simple" ("is
better" is implied) with a concrete point (i.e. By moving that to
the end, you removed the need to conditionally shift $@ in the
function to simplify the codepath), based on my _guess_ of what you
possibly meant to say, from reading your description that did not
give much clue for me to guess why you thought the result was "more
elegant". If my guess missed what your true justification was,
please replace it with the more correct one ;-)
>> I cannot read why you think the updated commit_msg is "more pretty"
>> in the message or in the patch.
>>
>> > -commit_msg () {
>> > - # String "initial. initial" partly in German (translated with Google
>> > Translate),
>> > +commit_msg() {
>>
>> Style. Have SP on both sides of () in a shell function definition.
> Could you point me to the coding style guide, please?
Documentation/CodingGuidelines::
- We prefer a space between the function name and the parentheses. The
opening "{" should also be on the same line.
E.g.: my_function () {
--
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 v9 3/5] t4205, t6006, t7102: make functions more readable
On Thu, Jul 04, 2013 at 11:45:57PM -0700, Junio C Hamano wrote:
> Alexey Shumkin writes:
>
> > Function 'test_format' is become hard to read after its change in
> > de6029a2d7734a93a9e27b9c4471862a47dd8123. So, make it more elegant.
> > Also, change 'commit_msg' function to make it more pretty.
>
> I do not know where you pick up these "more elegant" and "more
> pretty" from, but please refrain from using _only_ such vague and
> subjective phrases to describe the change in the log message.
> Saying "make it <> by doing X" (with various subjective
> adjectives to say "better") is fine, but make sure you have "doing
> X" part in the explanation.
>
> Perhaps like this.
>
> Function 'test_format' has become harder to read after its
> change in de6029a2 (pretty: Add failing tests: --format output
> should honor logOutputEncoding, 2013-06-26). Simplify it by
> moving its "should we expect it to fail?" parameter to the end.
I'm not sure whether this "last parameter" is needed in that code as far as we
already removed expected to fail tests
>
> I cannot read why you think the updated commit_msg is "more pretty"
> in the message or in the patch.
>
> > -commit_msg () {
> > - # String "initial. initial" partly in German (translated with Google
> > Translate),
> > +commit_msg() {
>
> Style. Have SP on both sides of () in a shell function definition.
Could you point me to the coding style guide, please?
>
> > + # String "initial. initial" partly in German
> > + # (translated with Google Translate),
> > # encoded in UTF-8, used as a commit log message below.
> > msg=$(printf "initial. anf\303\244nglich")
> > if test -n "$1"
>
> This is not "more pretty" but "better commented".
Well, this is "better formatted comment", I guess :)
>
> > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> > index 2ef96e9..73a1bdb 100755
> > --- a/t/t7102-reset.sh
> > +++ b/t/t7102-reset.sh
> > @@ -9,15 +9,17 @@ Documented tests for git reset'
> >
> > . ./test-lib.sh
> >
> > -commit_msg () {
> > - # String "modify 2nd file (changed)" partly in German(translated with
> > Google Translate),
> > +commit_msg() {
> > + # String "modify 2nd file (changed)" partly in German
> > + # (translated with Google Translate),
> > # encoded in UTF-8, used as a commit log message below.
> > - msg=$(printf "modify 2nd file (ge\303\244ndert)")
> > + printf "modify 2nd file (ge\303\244ndert)" |
> > if test -n "$1"
> > then
> > - msg=$(echo $msg | iconv -f utf-8 -t $1)
> > + iconv -f utf-8 -t $1
> > + else
> > + cat
> > fi
> > - echo $msg
>
> Is it "more pretty"? The "we have to have cat only because we want
> to pipe into a conditional" look somewhat ugly.
That was a proposition of J6t :-D
(see http://article.gmane.org/gmane.comp.version-control.git/229291):
>If you wanted to, you could write this as
>
>commit_msg () {
># String "modify 2nd file (changed)" partly in German
>#(translated with Google Translate),
># encoded in UTF-8, used as a commit log message below.
>printf "modify 2nd file (ge\303\244ndert)" |
>if test -n "$1"
>then
>iconv -f utf-8 -t $1
>else
>cat
>fi
>}
>
>but I'm not sure whether it's a lot better.
Last sentence has apperared to be a key
>
> msg="modify 2nd file (ge\303\244ndert)"
> if test -n "$1"
> then
> printf "$msg" | iconv -f utf-8 -t "$1"
> else
> printf "$msg"
> fi
>
> > }
> >
> > test_expect_success 'creating initial files and commits' '
--
Alexey Shumkin
--
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 v9 3/5] t4205, t6006, t7102: make functions more readable
Alexey Shumkin writes:
> Function 'test_format' is become hard to read after its change in
> de6029a2d7734a93a9e27b9c4471862a47dd8123. So, make it more elegant.
> Also, change 'commit_msg' function to make it more pretty.
I do not know where you pick up these "more elegant" and "more
pretty" from, but please refrain from using _only_ such vague and
subjective phrases to describe the change in the log message.
Saying "make it <> by doing X" (with various subjective
adjectives to say "better") is fine, but make sure you have "doing
X" part in the explanation.
Perhaps like this.
Function 'test_format' has become harder to read after its
change in de6029a2 (pretty: Add failing tests: --format output
should honor logOutputEncoding, 2013-06-26). Simplify it by
moving its "should we expect it to fail?" parameter to the end.
I cannot read why you think the updated commit_msg is "more pretty"
in the message or in the patch.
> -commit_msg () {
> - # String "initial. initial" partly in German (translated with Google
> Translate),
> +commit_msg() {
Style. Have SP on both sides of () in a shell function definition.
> + # String "initial. initial" partly in German
> + # (translated with Google Translate),
> # encoded in UTF-8, used as a commit log message below.
> msg=$(printf "initial. anf\303\244nglich")
> if test -n "$1"
This is not "more pretty" but "better commented".
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 2ef96e9..73a1bdb 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -9,15 +9,17 @@ Documented tests for git reset'
>
> . ./test-lib.sh
>
> -commit_msg () {
> - # String "modify 2nd file (changed)" partly in German(translated with
> Google Translate),
> +commit_msg() {
> + # String "modify 2nd file (changed)" partly in German
> + # (translated with Google Translate),
> # encoded in UTF-8, used as a commit log message below.
> - msg=$(printf "modify 2nd file (ge\303\244ndert)")
> + printf "modify 2nd file (ge\303\244ndert)" |
> if test -n "$1"
> then
> - msg=$(echo $msg | iconv -f utf-8 -t $1)
> + iconv -f utf-8 -t $1
> + else
> + cat
> fi
> - echo $msg
Is it "more pretty"? The "we have to have cat only because we want
to pipe into a conditional" look somewhat ugly.
msg="modify 2nd file (ge\303\244ndert)"
if test -n "$1"
then
printf "$msg" | iconv -f utf-8 -t "$1"
else
printf "$msg"
fi
> }
>
> test_expect_success 'creating initial files and commits' '
--
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
[PATCH v9 3/5] t4205, t6006, t7102: make functions more readable
Function 'test_format' is become hard to read after its change in
de6029a2d7734a93a9e27b9c4471862a47dd8123. So, make it more elegant.
Also, change 'commit_msg' function to make it more pretty.
Signed-off-by: Alexey Shumkin
Improved-by: Johannes Sixt
---
t/t4205-log-pretty-formats.sh | 5 +++--
t/t6006-rev-list-format.sh| 23 +--
t/t7102-reset.sh | 12 +++-
3 files changed, 15 insertions(+), 25 deletions(-)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ef9770a..bb87f02 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -7,8 +7,9 @@
test_description='Test pretty formats'
. ./test-lib.sh
-commit_msg () {
- # String "initial. initial" partly in German (translated with Google
Translate),
+commit_msg() {
+ # String "initial. initial" partly in German
+ # (translated with Google Translate),
# encoded in UTF-8, used as a commit log message below.
msg=$(printf "initial. anf\303\244nglich")
if test -n "$1"
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 4751d22..e069263 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -35,26 +35,13 @@ test_expect_success 'setup' '
git config --unset i18n.commitEncoding
'
-# usage: test_format [failure] name format_string expect.$1
- name="format $1"
- command="git rev-list --pretty=format:'$2' master >output.$1 &&
- test_cmp expect.$1 output.$1"
- if test $must_fail -eq 1
- then
- test_expect_failure "$name" "$command"
- else
- test_expect_success "$name" "$command"
- fi
+ test_expect_${3:-success} "format $1" "
+ git rev-list --pretty=format:'$2' master >output.$1 &&
+ test_cmp expect.$1 output.$1
+ "
}
# Feed to --format to provide predictable colored sequences.
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 2ef96e9..73a1bdb 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -9,15 +9,17 @@ Documented tests for git reset'
. ./test-lib.sh
-commit_msg () {
- # String "modify 2nd file (changed)" partly in German(translated with
Google Translate),
+commit_msg() {
+ # String "modify 2nd file (changed)" partly in German
+ # (translated with Google Translate),
# encoded in UTF-8, used as a commit log message below.
- msg=$(printf "modify 2nd file (ge\303\244ndert)")
+ printf "modify 2nd file (ge\303\244ndert)" |
if test -n "$1"
then
- msg=$(echo $msg | iconv -f utf-8 -t $1)
+ iconv -f utf-8 -t $1
+ else
+ cat
fi
- echo $msg
}
test_expect_success 'creating initial files and commits' '
--
1.8.3.1.15.g5c23c1e
--
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
