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

Add GeoBoundingBox filter #84

Merged

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Sep 17, 2024

Why Loupe already supports filtering by a GeoDistance. Which support we already merged also int SEAL. It would also be nice to filter by a bounding box (rectangle). That is mostly used for things when a user navigates around a map and have a specific area shown.

See fore more information: #83

fixes #83

Implemented as far as I could get with my knowledge.

tests/Functional/SearchTest.php Outdated Show resolved Hide resolved
src/Internal/Filter/Ast/GeoBoundingBox.php Outdated Show resolved Hide resolved
Copy link
Contributor

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Implemented as far as I could get with my knowledge.

Do you want me to take over from here or do you want to give it a shot with my feedback? You are almost there!

src/Internal/Filter/Ast/GeoBoundingBox.php Outdated Show resolved Hide resolved
tests/Functional/SearchTest.php Outdated Show resolved Hide resolved
src/Internal/Filter/Parser.php Outdated Show resolved Hide resolved
@alexander-schranz alexander-schranz marked this pull request as ready for review September 18, 2024 08:00
@alexander-schranz
Copy link
Contributor Author

@Toflar ready for review

Copy link
Contributor

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Awesome! You have implemented almost all that's needed, great!!

src/Internal/Search/Searcher.php Show resolved Hide resolved
tests/Functional/SearchTest.php Outdated Show resolved Hide resolved
tests/Unit/Internal/Filter/ParserTest.php Show resolved Hide resolved
src/Internal/Filter/Ast/GeoBoundingBox.php Outdated Show resolved Hide resolved
Copy link
Contributor

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Just one more test I think would be very helpful.
This looks very cool - how do you like it? :)

'_geoBoundingBox(location, 1.00 2.00, 2.00, 3.00)',
"Col 31: Error: Expected ',', got '2.00'",
];

Copy link
Contributor

Choose a reason for hiding this comment

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

Just one more test for invalid coordinates would be great :)

@Toflar
Copy link
Contributor

Toflar commented Sep 18, 2024

Oh and some docs on how to use the filter https://github.com/loupe-php/loupe/blob/main/docs/searching.md#filter

Copy link
Contributor

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Some docs improvements and then I think this is ready to be merged. Awesome work, Alex!

docs/searching.md Outdated Show resolved Hide resolved
docs/searching.md Outdated Show resolved Hide resolved
docs/searching.md Outdated Show resolved Hide resolved
docs/searching.md Outdated Show resolved Hide resolved
@Toflar Toflar merged commit 54c0548 into loupe-php:main Sep 18, 2024
14 checks passed
@Toflar
Copy link
Contributor

Toflar commented Sep 18, 2024

Thanks @alexander-schranz! This is very cool! I'm going to clean up some stuff and then release it as v0.7 :)

@alexander-schranz alexander-schranz deleted the feature/geo-bounding-box-filter branch September 18, 2024 12:18
@alexander-schranz
Copy link
Contributor Author

@Toflar Awesome. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for filtering by a GeoBoundingBox
2 participants