Skip to content

Add withinPolygon to Query #684

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

Merged
merged 6 commits into from
Aug 16, 2017
Merged

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jun 21, 2017

parse-community/parse-server#3866

I haven't used java in a while so any help will be great

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #684 into master will increase coverage by 0.03%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #684      +/-   ##
============================================
+ Coverage     53.02%   53.06%   +0.03%     
- Complexity     1726     1732       +6     
============================================
  Files           132      132              
  Lines         10240    10254      +14     
  Branches       1432     1432              
============================================
+ Hits           5430     5441      +11     
- Misses         4363     4364       +1     
- Partials        447      449       +2
Impacted Files Coverage Δ Complexity Δ
Parse/src/main/java/com/parse/ParseQuery.java 74.47% <100%> (+0.29%) 83 <2> (+2) ⬆️
...rse/src/main/java/com/parse/OfflineQueryLogic.java 63.54% <87.5%> (+0.46%) 115 <2> (+4) ⬆️
...se/src/main/java/com/parse/ParseKeyValueCache.java 45% <0%> (-2%) 16% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 351d54b...ed3ff26. Read the comment docs.

@Jawnnypoo
Copy link
Member

@flovilmart Do we support this on iOS? Seems like something we would want parity on

@flovilmart
Copy link
Contributor

The PR is open on the iOS SDK as well, we're also trying to figure out how to keep track of SDK required changes properly (and even better automatically)

@addisonElliott
Copy link
Contributor

Why has this pull request not been merged?

@natario1
Copy link
Contributor

natario1 commented Aug 8, 2017

What's the status of this in other repos?

@dplewis would it be easy for you to add Local Datastore support? It should be a few lines additions in OfflineQueryLogic I think. There is also OfflineQueryLogicTest with samples. But I am not sure if it's an easy task, what do you think?

@dplewis
Copy link
Member Author

dplewis commented Aug 8, 2017

@natario1 @addisonElliott withinPolygon is in all other SDKs. For Local Datastore I need an efficient pointInPolygon function. I have some time to day to update the android sdk

@natario1
Copy link
Contributor

natario1 commented Aug 8, 2017

@dplewis that would be great! Thanks for the effort.

@natario1
Copy link
Contributor

@dplewis what do you think about refactoring whereWithinPolygon to accept a ParsePolygon? I think it would be better now, not sure if you already planned

@dplewis
Copy link
Member Author

dplewis commented Aug 15, 2017

@natario1 Do you mean something like this?

public ParseQuery<T> whereWithinPolygon(String key, Object value) {
    ParsePolygon polygon = null;

    if (value instanceof ParsePolygon) {
      polygon = (ParsePolygon)value;
    }
    if (value instanceof List) {
      polygon = new ParsePolygon((List<ParseGeoPoint>)value);
    }
    builder.whereGeoWithin(key, polygon.getCoordinates());
    return this;
  }

@natario1
Copy link
Contributor

@dplewis no I mean just public ParseQuery<T> whereWithinPolygon(String key, ParsePolygon value). Or we could keep both:

public ParseQuery<T> whereWithinPolygon(String key, ParsePolygon value) {
    ...
}

public ParseQuery<T> whereWithinPolygon(String key, List<ParseGeoPoint> value) {
    return whereWithinPolygon(new ParsePolygon(value)); // or the other way
}

Also the ParseQuery.Builder method could accept a polygon now that we have the model, instead of a list of coordinates? This way you could also avoid instantiating a new polygon just for query matching here. But yeah this is less important.

@dplewis
Copy link
Member Author

dplewis commented Aug 15, 2017

I will change it to support both.

The query builder will still have to use a list of coordinates for now until this PR goes through parse-community/parse-server#4067

@natario1 natario1 merged commit 28fd9e1 into parse-community:master Aug 16, 2017
@natario1
Copy link
Contributor

Great... thanks man!

We should add docs for this and #696 some day

@flovilmart
Copy link
Contributor

flovilmart commented Aug 16, 2017 via email

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.

5 participants