Re: [PATCH v9 3/5] t4205, t6006, t7102: make functions more readable
Alexey Shumkin alex.crez...@gmail.com 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 better 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
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 alex.crez...@gmail.com 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 better 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
On Fri, Jul 05, 2013 at 01:44:07AM -0700, Junio C Hamano wrote:
Alexey Shumkin alex.crez...@gmail.com 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 alex.crez...@gmail.com 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
[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 alex.crez...@gmail.com
Improved-by: Johannes Sixt j.s...@viscovery.net
---
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 expected_output
+# usage: test_format name format_string [failure] expected_output
test_format () {
- must_fail=0
- # if parameters count is more than 2 then test must fail
- if test $# -gt 2
- then
- must_fail=1
- # remove first parameter which is flag for test failure
- shift
- fi
cat 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
