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

Added HasGeometryScropes trait and a sample implementation of within #86

Closed
wants to merge 1 commit into from
Closed

Added HasGeometryScropes trait and a sample implementation of within #86

wants to merge 1 commit into from

Conversation

ajthinking
Copy link
Contributor

Hi,
I was hoping to contribute to this repository. Could you please tell me if you like this structure?
If so I can add some more functions and make a test suite for them.

The scopes design is inspired from: https://github.com/grimzy/laravel-mysql-spatial/blob/master/src/Eloquent/SpatialTrait.php

I figured it would be nice to put these scopes in a separate file, or what do you think?

Also, pondered, what if scopes could accept strings like 'App\Park', tablenames, instance id or something else to make the actual queries inside of the database. To use the scopes with a WKT insertion does not seem optimal? Fine if it is some BBOX, but what if it has thousands of vertexes. Any guidance of these kind of questions would help alot.

Finally please be gentle as this is my first serious PR :)

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-0.7%) to 83.239% when pulling a1e1cdf on ajthinking:HasGeometryScopes into 16e77b8 on njbarrett:master.

@njbarrett
Copy link
Collaborator

HI @ajthinking - I think the style of the provided PR is fine, if you can write tests for it.
I actually think we need to allow you to scopeWithin another column of the database.
I am doing something similar on a seperate branch for a project where I needed this: https://github.com/njbarrett/laravel-postgis/blob/add-query-scopes/src/Eloquent/PostgisTrait.php#L79

However yours is a little more robust. So as long as its possible to reference a column as a point and a column as bounds then I would be happy with it.

@ajthinking
Copy link
Contributor Author

Cool, I will get back here when tests are ready. And yes, I found that branch just as I wrapped up, but that was marked as stalled so I figured it was not to be merged (?).

I actually think we need to allow you to scopeWithin another column of the database.

Ill think about how that is done, ideally the parameter should support the geometry classes as well as other db columns. Will problably need some pointers from you or others. Can I use this PR and refresh it by commiting to my branch? Or should we do a new one?

@njbarrett
Copy link
Collaborator

You can keep committing to this branch.
And yeah the other branch I was working on for this wasnt intended for merging, just needed for a specific project but id like to get a robust solution in.

@mstaack mstaack closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants