Define in-test function at the head of test #4932
Conversation
Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>
Warashi
requested review from
nghialv,
khanhtc1202,
kentakozuka,
ffjlabo and
t-kikuc
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4932 +/- ##
==========================================
+ Coverage 29.35% 29.39% +0.03%
==========================================
Files 322 322
Lines 40852 40852
==========================================
+ Hits 11994 12010 +16
+ Misses 27897 27882 -15
+ Partials 961 960 -1 ☔ View full report in Codecov by Sentry. |
| getGCAutoDetach := func(ctx context.Context, repo *repo) (bool, error) { | ||
| cmd := exec.CommandContext(ctx, repo.gitPath, "config", "--get", "gc.autoDetach") | ||
| cmd.Dir = repo.dir | ||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| v, err := strconv.ParseBool(strings.TrimSuffix(string(out), "\n")) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return v, nil | ||
| } |
There was a problem hiding this comment.
TBH, I prefer to push the variable/function declaration right before placing it to use, which increases the readability. The closure function outer variable shadowing or reference is a normal thing we have to face if we use it, so I think it's better to capture it in the review process rather than placing it at the head of the function. wdyt?
Happy to hear other opinions on this cc @ffjlabo @t-kikuc
There was a problem hiding this comment.
IMO, there is a trade-off between the readability of the closure and the codes that use the closure.
When I read the closure itself, closures placed at the top has more readability because there is no captured variable.
On the other hand, when I read codes that use the closure, places at just before it is used has more readability.
In this case, it's not so far between the top of the test function and the line it uses because test is 55 lines of code, not so long.
I think it is short enough to switch eye between definition and call.
There was a problem hiding this comment.
Okey, this is just a small change, let's choose the win-win option 🙌
Warashi
changed the title
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
What this PR does / why we need it:
This PR moves the definition of the in-test function to the top of the test.
This makes the in-test function unable to capture variables other than
*testing.T.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: No