New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor readabilty improvements #713

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
3 participants
@leitzler
Contributor

leitzler commented Oct 17, 2017

No description provided.

}
return nil
err := s.BuildFiles(args, pkgObj, currentDirectory)
return err

This comment has been minimized.

@flimzy

flimzy Oct 17, 2017

Contributor

If you're going to make the change, why not go all the way to:

return s.BuildFiles(args, pkgObj, currentDirectory)
@flimzy

flimzy Oct 17, 2017

Contributor

If you're going to make the change, why not go all the way to:

return s.BuildFiles(args, pkgObj, currentDirectory)

This comment has been minimized.

@leitzler

leitzler Oct 17, 2017

Contributor

Imo it's more readable to see that it returns an error, but it isn't that strong opinion :)

@leitzler

leitzler Oct 17, 2017

Contributor

Imo it's more readable to see that it returns an error, but it isn't that strong opinion :)

This comment has been minimized.

@dmitshur

dmitshur Oct 17, 2017

Member

That would work too, but personally, I don't mind (in fact, I slightly prefer) it as written. It has to do with where I perceive the emphasis to be when reading the code.

When I see:

err := s.BuildFiles(args, pkgObj, currentDirectory)
return err

I read that as "do s.BuildFiles, then return the error result."

When I see:

return s.BuildFiles(args, pkgObj, currentDirectory)

I read that as "return the result of building files", which seems to put more emphasis on the return value, but in fact the return value is pretty insignificant (it doesn't return a package it built, just the error).

Anyway, this is a personal style and both ways are fine.

@dmitshur

dmitshur Oct 17, 2017

Member

That would work too, but personally, I don't mind (in fact, I slightly prefer) it as written. It has to do with where I perceive the emphasis to be when reading the code.

When I see:

err := s.BuildFiles(args, pkgObj, currentDirectory)
return err

I read that as "do s.BuildFiles, then return the error result."

When I see:

return s.BuildFiles(args, pkgObj, currentDirectory)

I read that as "return the result of building files", which seems to put more emphasis on the return value, but in fact the return value is pretty insignificant (it doesn't return a package it built, just the error).

Anyway, this is a personal style and both ways are fine.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 17, 2017

Member

Thanks for the PR. I agree these changes are an improvement.

However, I remember seeing an exact same (or very similar) change in the past (edit: found it, see #596 (comment) and 564024c), and @neelance rejected it then, because he preferred the current style at the time.

However, it was a few years ago (edit: actually, only 8~ months ago), and I wouldn't be surprised if he's okay with this code simplification now. I think we should merge it, because the code is simpler, and it allows other people who read it not get distracted and want to send a PR just like this one.

Member

dmitshur commented Oct 17, 2017

Thanks for the PR. I agree these changes are an improvement.

However, I remember seeing an exact same (or very similar) change in the past (edit: found it, see #596 (comment) and 564024c), and @neelance rejected it then, because he preferred the current style at the time.

However, it was a few years ago (edit: actually, only 8~ months ago), and I wouldn't be surprised if he's okay with this code simplification now. I think we should merge it, because the code is simpler, and it allows other people who read it not get distracted and want to send a PR just like this one.

@dmitshur

LGTM.

I'll merge it after some time, unless @neelance has objections.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 2, 2017

Member

There've been no objections, and I think this is valid, idiomatic Go code that is slightly simpler. It'll avoid future people sending the same simplification. Merging. Thanks again!

Member

dmitshur commented Nov 2, 2017

There've been no objections, and I think this is valid, idiomatic Go code that is slightly simpler. It'll avoid future people sending the same simplification. Merging. Thanks again!

@dmitshur dmitshur merged commit 077f6d6 into gopherjs:master Nov 2, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment