Skip to content

Conversation

@muesli
Copy link
Contributor

@muesli muesli commented Mar 22, 2018

  • Added copyright 2015-2018 CoreOS, Inc.
  • Proper godoc package comment
  • Got rid of named (but unused) return values

Fixes #83.

lucab
lucab previously requested changes Mar 26, 2018
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor stylistic comments, all the rest is 💚.

if err != nil {
return false, err
err := os.Unsetenv("NOTIFY_SOCKET")
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge this and the above in a single statement.

return false, err
}
return true, nil
return err == nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep logic outside of return statement.
Even if I agree the original form may be overly verbose, I think it is the expected idiomatic form.

- Added copyright 2015-2018 CoreOS, Inc.
- Proper godoc package comment
- Got rid of named (but unused) return values

Fixes coreos#83.
@muesli
Copy link
Contributor Author

muesli commented Mar 26, 2018

@lucab Done and squashed.

@lucab lucab dismissed their stale review March 27, 2018 07:44

stale

Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucab lucab merged commit 1ae4dd7 into coreos:master Mar 27, 2018
@lucab lucab added this to the v17 milestone May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants