Skip to content
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

Fix function comments based on best practices from Effective Go #75

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@CodeLingoTeam
Copy link

commented Mar 1, 2019

Hi, we updated some exported function comments based on best practices from Effective Go. It’s admittedly a relatively minor fix up. Does this help you?

Fix function comments based on best practices from Effective Go
Signed-off-by: CodeLingo Bot <bot@codelingo.io>
@dmitshur
Copy link
Member

left a comment

Thanks for the PR, sorry about the delay.

The changes are good. I've made one minor improvement suggestion, see inline comments.

But this is an improvement and would work as is.

@@ -212,7 +212,7 @@ func BezierSurface(u, v float32, cPoints [][]Vec3) Vec3 {
return point
}

// Does interpolation over a spline of several bezier curves. Each bezier curve must have a finite range,
// BezierSplineInterpolate2D Does interpolation over a spline of several bezier curves. Each bezier curve must have a finite range,

This comment has been minimized.

Copy link
@dmitshur

dmitshur Apr 11, 2019

Member

Can you change "Does" to "does" here, and everywhere similar below?

@@ -186,7 +186,7 @@ func (v Vec2) Y() float32 {
return v[1]
}

// Does the vector outer product
// OuterProd2 Does the vector outer product

This comment has been minimized.

Copy link
@dmitshur

dmitshur Apr 11, 2019

Member

Here and below as well.

@@ -347,7 +347,7 @@ func (v Vec3) Z() float32 {
return v[2]
}

// Does the vector outer product
// OuterProd2 Does the vector outer product

This comment has been minimized.

Copy link
@dmitshur

dmitshur Apr 11, 2019

Member

Here and below, too.

@pwaller

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@dmitshur judging from the user's activity, it is a bot, so I think we'll have to make the fix ourselves.

@CodeLingoTeam @CodeLingoBot if you're listening, I think @dmitshur's suggestion would apply as a general rule, if the bot could be fixed.

Also interesting that it looks like our travis configuration is broken. I'll make a new PR and try and fix both.

@pwaller

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

There is another problem with these changes, they're actually changing generated code. See the header:

// This file is generated by codegen.go; DO NOT EDIT
// Edit vector.tmpl and run "go generate" to make changes.

@CodeLingoTeam @CodeLingoBot as per https://golang.org/s/generatedcode - these files should not have been edited anyway. Closing this PR, I'll see about submitting fixes to the underlying templates.

@pwaller pwaller closed this Apr 11, 2019

pwaller added a commit that referenced this pull request Apr 11, 2019

Fix docstrings to match effective Go convention
Further to pull request in #75.

Turned out to be a surprisingly large amount of low value work :(. It's done now I suppose.

This was done on a best effort basis. I've reviewed the patch and don't think there are any non-comment/whitespace changes present. I have verified this by grepping for `^\+[^/]` which looks for diff lines not starting with /.

There was one minor thing where I reordered a small function to make it easier to edit the comments in bulk.
@pwaller

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@CodeLingoTeam I have some feedback on your bot and this PR.

This did cost me an hour of work to fix in the end, and I guess it is fairly low value improvement.

It would be useful if you made it clear in the pull request that this is a bot making the pull request, and it would also have been useful if you had included a link to a way to reproduce your automated changes, since I ended up redoing them due to the templating involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.