Conversation
|
Looks good to me. There's a lot of old Go code of mine in here written before I understood the popular Go conventions. |
commands/commands.go
Outdated
There was a problem hiding this comment.
If we're going to go with this style of import block (which I am in favor of, and is the only annoying difference between goimports and go fmt), then we should do it this way everywhere.
|
👍 for me, other than hoping to see those import blocks consistent across the code base. |
|
Thanks for the feedback. I'll change the imports. |
|
I don't think the imports are necessary to change in this PR. Instead, I think we should setup a separate PR that somehow bundles |
|
I think this PR should leave them consistent, whichever way we go. I'm not entirely convinced bundling goimports is necessary, go fmt will honor the spaced blocks if it encounters them. I could be persuaded otherwise, though. I honestly don't use script/fmt and just run that stuff from my editor. |
|
Actually, you do use script/fmt, it's called during script/test :) I'm totally cool with consistent imports in this PR. On Tue, May 12, 2015 at 10:25 AM, Scott Barron notifications@github.com
Rick Olson |
Decreases nesting
I needed some extra time to understand what was happening. In my opinion, the control flow is easier to follow when checking for err != nil, as this is the convention in go.
|
I removed the import reformatting from this PR. (That's what I meant with “I'll change the imports.”) |
|
Thanks! I think we'll just do that in an isolated PR. |
This is my first contribution to this project, so I'm not sure what's appreciated and what's not. Please let me know. I'm happy to change the PR in any way necessary.
As I worked myself through the codebase I made some changes to the codestyle and did some minor refactoring (switched if-else; returned nil as payload with error). I split it in small commits to make it easier to follow.