Add ParsePolygon Type and polygonContains query #327

dplewis · 2017-06-23T04:58:02Z

Added ParsePolygon Type created from Array of Coordinates or ParseGeoPoints
Added polygonContains to query.

I'm using PHP as a base for updating the other sdks.

Let me know if any more tests are needed or additional functionality.

montymxb

Overall looks good! Always cool to see new functionality coming to parse 👍 .

I'll wait for the requested changes and we'll go from there. Once the PR is merged in on the server I'll trigger Travis to rerun the test cases to see how that looks.

montymxb · 2017-06-23T17:01:00Z

src/Parse/ParsePolygon.php

+     */
+    public function setCoordinates($coords)
+    {
+        if (count($coords) < 3 || !is_array($coords)) {


Can you move the array exception here into a separate check just above? It's close but the user should be informed that an array of coordinates is required when one is missing, just to be more specific about which is which.

Thanks, I'll get on it

montymxb · 2017-06-23T17:03:53Z

src/Parse/ParsePolygon.php

+        $points = [];
+        foreach ($coords as $coord) {
+            $geoPoint = $coord;
+            if (is_array($coord)) {


Since you're only adding this coordinate set if it's an array we should probably add another check here instead of silently skipping entries. Something that expects $coord to not only be an array, but to also contain a lat and long.

montymxb · 2017-06-23T17:08:54Z

src/Parse/ParsePolygon.php

+            if (is_array($coord)) {
+                $geoPoint = new ParseGeoPoint($coord[0], $coord[1]);
+            }
+            $points[] = [$geoPoint->getLatitude(), $geoPoint->getLongitude()];


A note that currently this would fail if you pass something that's not an array by accident, like a string, as it would try to run $geoPoint->get... on an object that's not necessarily a GeoPoint. If you add the check mentioned just above this shouldn't be a problem anymore though, as you would be able to safely create a new ParseGeoPoint and add it after the check.

It looks like you're expecting users to either supply an array of coords or a ParseGeoPoint instance. To that end you could add an additional check as an else if to ensure that if the object is not an array it is at least an instance of ParseGeoPoint. If $coords is neither of those it would be safe to throw an exception stating that one or the other is required to proceed.

montymxb · 2017-06-23T17:12:25Z

tests/Parse/ParsePolygonTest.php

+        $obj->set('polygon', $polygon);
+        $obj->save();
+    }
+


Assuming you add the aforementioned exceptions to further scrutinize the input to ParsePolygon, you would have to add a couple cases here for firing the other conditions.

dplewis · 2017-06-23T23:20:55Z

There's an added benefit of mixing and matching points and geopoints

montymxb

This looks good to me! Thanks for putting all this up.

I'll leave this on ice for the moment until the pending functionality is in place server-side ☃️. Once that's done we can rerun the CI and see how it goes. Assuming everything comes out good I'll approve the changes and we'll move onto getting this added in!

montymxb · 2017-06-24T01:18:12Z

src/Parse/ParsePolygon.php

        }
        $points = [];
        foreach ($coords as $coord) {
-            $geoPoint = $coord;
-            if (is_array($coord)) {
+            $geoPoint;


Don't really need this here, lexical scoping doesn't apply in this case for php.

However...with that said I imagine some IDEs would like to highlight later uses as 'potentially undefined', even though the final case is throwing an exception. With that in mind this is okay to leave here.

Wow I didn't know that. Thanks for the knowledge.

montymxb · 2017-07-12T03:02:39Z

@dplewis looks like #3944 is pretty much good to go! Once that gets merged in we can move along with this.

flovilmart · 2017-07-12T03:33:59Z

now it's merged!

* Add lint * update travis * coding style * add coverage to gitignore * removed ignore lines * nit

* Corrects and updates phpdoc references/errors * Lint fixes

Adds a **Getting Started** section to README.md to direct newcomers to the [official guide](http://docs.parseplatform.org/php/guide/) and [API reference](http://parseplatform.org/parse-php-sdk/namespaces/Parse.html).

* Removed default api and added appropriate checking * lint

* Added 'document-check' to add phpdoc checking during tests and added deploy for api ref on gh-pages * Wrapping filename in quotes * Moved bash out of package.json * Unescaping strings * Testing missing block comment * Fixing lint exception to expose phpdoc style issue * Restored class summary * removed comment

montymxb · 2017-07-12T04:00:48Z

Whoops looks like the latest branch only just finished releasing. I'll rerun these once more to get that change.

montymxb · 2017-07-12T06:29:29Z

@dplewis tests look good! What's causing this to fail out is the phpdoc enforcing that was added not too long ago. You can verify that phpdoc looks good via npm run document-check locally to verify this.

In this case it seems you need a summary for ParsePolygon.php. You can check this against the other src files, like the top of ParseACL.php for example.

<?php
/**
 * Class ParseACL | Parse/ParseACL.php
 */

dplewis · 2017-07-12T17:24:27Z

@montymxb I figured something like that would happen. I fixed the summary documentation.

montymxb · 2017-07-12T17:38:04Z

@dplewis thanks for the changes!

Looks like phpdoc is running on the contents of vendor. I'm seeing this on #334 as well, last night seemed to be good though. I'll see if I can't correct this and then we can retry here.

dplewis · 2017-07-12T17:44:13Z

@montymxb no problem

montymxb · 2017-07-12T17:46:54Z

Looks like updating composer dependencies allowed me to replicate this locally. It would appear a package has been updated between yesterday and today that is tripping this :/ . I'll keep digging and see how we can patch this up.

montymxb · 2017-07-12T18:03:39Z

@dplewis the culprit is jms/serializer, referenced internally by phpdoc.

It was updated to 1.8.0 earlier this morning. Locally specifying the prior 1.7.1 corrects this issue like so in composer.json under require-dev:

"jms/serializer": "1.7.1"

I'll put up a separate PR to fix this and merge it in real quick, then we can go from there.

* Pinned jms/serializer to 1.7.1 * Checking to update jms/serializer to 1.8.0 ONLY on php 5.4 for travis-ci * Added comment, and added graphviz for class diagrams in generated api docs

codecov · 2017-07-12T18:45:36Z

Codecov Report

Merging #327 into master will increase coverage by 0.74%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   98.04%   98.78%   +0.74%     
==========================================
  Files          35       35              
  Lines        2708     2727      +19     
==========================================
+ Hits         2655     2694      +39     
+ Misses         53       33      -20

Impacted Files	Coverage Δ
src/Parse/ParseRelation.php	`100% <ø> (ø)`	⬆️
src/Parse/ParseSession.php	`100% <ø> (ø)`	⬆️
src/Parse/ParseAggregateException.php	`100% <ø> (ø)`	⬆️
src/Parse/Internal/SetOperation.php	`100% <ø> (ø)`	⬆️
src/Parse/Internal/AddUniqueOperation.php	`100% <ø> (ø)`	⬆️
src/Parse/ParseACL.php	`100% <ø> (ø)`	⬆️
src/Parse/ParseInstallation.php	`100% <ø> (ø)`	⬆️
src/Parse/ParseSchema.php	`97.07% <ø> (ø)`	⬆️
src/Parse/ParseSessionStorage.php	`100% <ø> (ø)`	⬆️
src/Parse/Internal/AddOperation.php	`100% <ø> (ø)`	⬆️
... and 26 more

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 20dd7a5...699f9ab. Read the comment docs.

montymxb · 2017-07-12T18:57:06Z

Finally, alright I'll give this just another quick check and we should be good to go!

montymxb · 2017-07-12T19:01:41Z

@dplewis nearly 3 weeks later but it's finally in 👍 . Looks like you can attend to those other sdks as needed now.

Thanks again for adding in this functionality!

montymxb · 2017-07-12T19:09:35Z

Since each update to the master is now automatically deployed to gh-pages you can take a look at your api ref for ParsePolygon here. In addition there's a pretty cool class inheritance diagram you can check out as well.

dplewis · 2017-07-12T19:11:54Z

That diagram looks pretty cool

dplewis added 2 commits June 22, 2017 23:54

Add ParsePolygon Type and polygonContains query

eb52313

test class fix

72cd0a3

montymxb suggested changes Jun 23, 2017

View reviewed changes

error handling for polygon

46b4bd8

montymxb reviewed Jun 24, 2017

View reviewed changes

dplewis and others added 6 commits July 11, 2017 22:41

Update PHP_CodeSniffer (parse-community#328)

28b5e52

* Add lint * update travis * coding style * add coverage to gitignore * removed ignore lines * nit

Updated parse-php-sdk to version 1.2.9

2dbacd5

Corrects and updates phpdoc references/errors (parse-community#329)

ec69ddc

* Corrects and updates phpdoc references/errors * Lint fixes

Added 'Getting Started' to README.md

7b098e7

Adds a **Getting Started** section to README.md to direct newcomers to the [official guide](http://docs.parseplatform.org/php/guide/) and [API reference](http://parseplatform.org/parse-php-sdk/namespaces/Parse.html).

Remove Default API (parse-community#332)

9b40767

* Removed default api and added appropriate checking * lint

fix documentation

7e005f8

Pinned jms/serializer to 1.7.1 (parse-community#336)

699f9ab

* Pinned jms/serializer to 1.7.1 * Checking to update jms/serializer to 1.8.0 ONLY on php 5.4 for travis-ci * Added comment, and added graphviz for class diagrams in generated api docs

montymxb approved these changes Jul 12, 2017

View reviewed changes

montymxb merged commit 75f1199 into parse-community:master Jul 12, 2017

dplewis deleted the polygonContain branch July 12, 2017 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ParsePolygon Type and polygonContains query #327

Add ParsePolygon Type and polygonContains query #327

dplewis commented Jun 23, 2017

montymxb left a comment

montymxb Jun 23, 2017

dplewis Jun 23, 2017 •

edited

Loading

montymxb Jun 23, 2017

montymxb Jun 23, 2017

montymxb Jun 23, 2017

dplewis commented Jun 23, 2017

montymxb left a comment

montymxb Jun 24, 2017

dplewis Jun 24, 2017

montymxb commented Jul 12, 2017

flovilmart commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017 •

edited

Loading

dplewis commented Jul 12, 2017

montymxb commented Jul 12, 2017 •

edited

Loading

dplewis commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

codecov bot commented Jul 12, 2017 •

edited

Loading

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

dplewis commented Jul 12, 2017

Add ParsePolygon Type and polygonContains query #327

Add ParsePolygon Type and polygonContains query #327

Conversation

dplewis commented Jun 23, 2017

montymxb left a comment

Choose a reason for hiding this comment

montymxb Jun 23, 2017

Choose a reason for hiding this comment

dplewis Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

montymxb Jun 23, 2017

Choose a reason for hiding this comment

montymxb Jun 23, 2017

Choose a reason for hiding this comment

montymxb Jun 23, 2017

Choose a reason for hiding this comment

dplewis commented Jun 23, 2017

montymxb left a comment

Choose a reason for hiding this comment

montymxb Jun 24, 2017

Choose a reason for hiding this comment

dplewis Jun 24, 2017

Choose a reason for hiding this comment

montymxb commented Jul 12, 2017

flovilmart commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017 • edited Loading

dplewis commented Jul 12, 2017

montymxb commented Jul 12, 2017 • edited Loading

dplewis commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

codecov bot commented Jul 12, 2017 • edited Loading

Codecov Report

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

montymxb commented Jul 12, 2017

dplewis commented Jul 12, 2017

dplewis Jun 23, 2017 •

edited

Loading

montymxb commented Jul 12, 2017 •

edited

Loading

montymxb commented Jul 12, 2017 •

edited

Loading

codecov bot commented Jul 12, 2017 •

edited

Loading