Fixed GeoBoundingBox BottomLeft args order #1610
Conversation
|
@pistatium Can you check if that fixes all cases as described in #1600? |
|
I see...
So this seems to be the correct order for Please let me know if there are any other assumptions that need to be made. |
|
Won't this fix break existing clients? |
| @@ -65,7 +65,7 @@ func (q *GeoBoundingBoxQuery) BottomRightFromGeoHash(bottomRight string) *GeoBou | |||
|
|
|||
| // BottomLeft position from longitude (left) and latitude (bottom). | |||
| func (q *GeoBoundingBoxQuery) BottomLeft(bottom, left float64) *GeoBoundingBoxQuery { | |||
| q.bottomLeft = []float64{bottom, left} | |||
| q.bottomLeft = []float64{left, bottom} | |||
There was a problem hiding this comment.
Clients, like me, already swapped this and are using this in production. This fix will break unsuspecting clients that think this is a bug fix. If this change is accepted then it would warrant a major version change.
There was a problem hiding this comment.
I think this falls into the bucket of a very unfortunate bug on my side. However, if you were to fix all of these bugs by creating a new major version, we would probably be way beyond version 100 now. So I think we will fix it in a minor version.
If you fixed it, you are probably aware of the bug. Thankfully, Go modules won't silently update your dependency to the latest version automatically. However, I would be very thankful for a way to post a warning when the Go module is updated, but I'm not aware of anything like that. Changelogs are good, but people generally don't read them. A final option would be to remove BottomLeft and replace it with e.g. ButtomLeft2 to make people aware of the issue, but that also seems rather awkward to me.
So: It will be a minor version update, and watch out when you update.
There was a problem hiding this comment.
I think awkward beats breaking backwards compatibility, but that's just my opinion. I'd opt to make a new exposed function and deprecate the old one. This will certainly be a headache for some one down the road otherwise. It's your repo, so your call.
There was a problem hiding this comment.
I second @chchaffin - deprecate the old function and fix the bug in a new function. This is technically a bug fix, but anyone still using the current, bugged version of this function will have swapped the order to compensate, so there's going to be a lot more breakage if this goes out as a minor or patch version.
Hello.
The order of BottomLeft arguments in GeoBoundingBoxQuery was swapped, which has been corrected.
before:
"bottom_left":[40.01,-71.12],"top_right":[-74.1,40.73]}after:
bottom_left":[-71.12,40.01],"top_right":[-74.1,40.73]}