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.

Sign up for GitHub

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

Jump to bottom

Fixed GeoBoundingBox BottomLeft args order #1610

Open
wants to merge 1 commit into
base: release-branch.v7
Choose a base branch
from
Open

Fixed GeoBoundingBox BottomLeft args order #1610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion search_queries_geo_bounding_box.go
View file
Open in desktop
Expand Up @@ -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}
Copy link

@chchaffin chchaffin May 3, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

return q
}

Expand Down
20 changes: 20 additions & 0 deletions search_queries_geo_bounding_box_test.go
View file
Open in desktop
Expand Up @@ -29,6 +29,26 @@ func TestGeoBoundingBoxQuery(t *testing.T) {
}
}

func TestGeoBoundingBoxQueryInverted(t *testing.T) {
q := NewGeoBoundingBoxQuery("pin.location")
q = q.TopRight(40.73, -74.1)
q = q.BottomLeft(40.01, -71.12)
q = q.Type("memory")
src, err := q.Source()
if err != nil {
t.Fatal(err)
}
data, err := json.Marshal(src)
if err != nil {
t.Fatalf("marshaling to JSON failed: %v", err)
}
got := string(data)
expected := `{"geo_bounding_box":{"pin.location":{"bottom_left":[-71.12,40.01],"top_right":[-74.1,40.73]},"type":"memory"}}`
if got != expected {
t.Errorf("expected\n%s\n,got:\n%s", expected, got)
}
}

func TestGeoBoundingBoxQueryWithGeoPoint(t *testing.T) {
q := NewGeoBoundingBoxQuery("pin.location")
q = q.TopLeftFromGeoPoint(GeoPointFromLatLon(40.73, -74.1))
Expand Down