-
Notifications
You must be signed in to change notification settings - Fork 20
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
golint fixes #7
base: master
Are you sure you want to change the base?
golint fixes #7
Conversation
@@ -106,7 +106,7 @@ func (c *clipper) compute(operation Op) Polygon { | |||
// by sweeping from left to right. | |||
S := sweepline{} | |||
|
|||
MINMAX_X := math.Min(subjectbb.Max.X, clippingbb.Max.X) | |||
XMINMAX := math.Min(subjectbb.Max.X, clippingbb.Max.X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe xMinMax
would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please make it xMinMax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm if that is the only change? I prefer to do them all at once. What about the other capitals such as UNION, DIFFERENCE, ...?
It can be: Capital words are allowed without underscore. I have no preference, so I can change it it whatever @akavel prefers. |
This is pretty minor, and I'm okay with the last 2. But I'm not sure why you'd want to use all capitals for the words "min" and "max". They're not acronyms, are they? |
In my own go code I also use only capitals for acronyms. Using capitals for constants is a left over from other languages such as Python. I just left capitals to stay 'nearest' to the original code. For example the source code also contains UNION, DIFFERENCE, etc... I would have changed them as well, but I have no idea what the author of the package wants. In that case I try to keep the difference as small as possible. If @akavel give his preference, I can fix it. Right now I don't want to inflate the commit count. |
@@ -43,6 +44,7 @@ func (p Point) Length() float64 { | |||
return math.Sqrt(p.X*p.X + p.Y*p.Y) | |||
} | |||
|
|||
// A Rectangle contains the points with Min.X <= X <= Max.X, Min.Y <= Y <= Max.Y. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if it's <= Max.X
and <= Max.Y
. Please comment here with how you verified that. Sorry I can't do this myself now, will try when I find time if you don't.
Please make all the changes I listed if you can, and I'll merge it then. If you can't, I'll do them at some (not well defined) point in future myself and merge too. Thanks. EDIT: sorry: please also add an EDIT: Iqbal seems to have not left an email, so please do it without one. If it's not too big a trouble for you to add a line with |
…hat may also change the sweep context. (akavel#7) Expand the resweep logic to resweep an endpoint if it divides the next or previous segment but remains undivided. Both types of divisions can alter the sweep context for the endpoint.
Only golint fixes. See also: https://golang.org/doc/effective_go.html#mixed-caps