Simplify if-else chains and spacing #2453
Conversation
|
LGTM |
|
Do we have tests covering this path? Sergiusz Urbaniak notifications@github.com schrieb am Di., 19. Apr. 2016,
|
|
I don't think we have tests. The closest we have about config rktKind=paths seems to be in https://github.com/coreos/rkt/blob/master/tests/rkt_stage1_loading_test.go#L545 |
46a5741 to
bbb8bf4
Compare
|
I didn't add tests since methods were not exposed, but the best action would be to extract config methods out and create a new testable package |
| @@ -181,7 +183,9 @@ func init() { | |||
|
|
|||
| func getTabOutWithWriter(writer io.Writer) *tabwriter.Writer { | |||
| aTabOut := new(tabwriter.Writer) | |||
|
|
|||
There was a problem hiding this comment.
Are you adding all these blank lines or is some tool you're using doing it?
There was a problem hiding this comment.
I'm adding a new lines to logically group statements suggested by Effective Go docs. It's hard to read when the code is cluttered.
There was a problem hiding this comment.
reference? I can't find anything (looking for "logic", "group",
"statements", "newlines") in https://golang.org/doc/effective_go.html - and
indeed all of the code examples there seem to suggest otherwise
On Tue, Apr 19, 2016 at 1:53 PM, Tamer TAS notifications@github.com wrote:
In rkt/rkt.go
#2453 (comment):@@ -181,7 +183,9 @@ func init() {
func getTabOutWithWriter(writer io.Writer) *tabwriter.Writer {
aTabOut := new(tabwriter.Writer)
+I'm adding a new lines to logically group statements suggested by Effective
Go docs.—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/coreos/rkt/pull/2453/files/bbb8bf451a4cabc67e74f03103df18b24a2a5219#r60216890
There was a problem hiding this comment.
I must be confusing it with another guide, but using blank spaces to logically grouping statements improves code quality. From the functions I've read, there are a lot of clustered statements. I believe blank spaces would help with readability and blank spaces are cheap :)
|
Nothing looks wrong with this to me (aside from the superfluous spacing which I'm unsure about), but I'm pushing back on the tests because sometimes the most innocuous-looking refactorings can introduce the most "fun" bugs... @tmrts would you mind exploring adding something? |
|
@jonboulle I agree with your statement completely (except your comments about spacing :). I'll try to refactor this package or add tests to it then extract it in another PR. |
bbb8bf4 to
8dbd7c1
Compare
|
Tests are merged PTAL |
Refactors the if-else chains for readability Fixes rkt#2421
8dbd7c1 to
b8ee0d6
Compare
|
LGTM |
|
really guys really |
Refactors the if-else chains for readability
Fixes #2421