Convert the input format from lng lat to lat lng #1480
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe coordinate order for polygon-related XPath functions and their documentation was changed from "longitude latitude" to "latitude longitude". Documentation comments and example usages were updated to reflect this, and test cases were revised and expanded to use the new coordinate order. No changes were made to implementation logic. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java (1)
26-28: Parameter order in Javadoc contradicts both syntax line and implementation
is-point-inside-polygon(point_coord, polygon_coords)(line 21) – and the method body – expect the point first, polygon second.
The list below reverses them (polygon_coords, thenpoint_coord). This will mislead API users.- * <li><code>polygon_coords</code>: A space-separated string of lat/lon pairs …</li> - * <li><code>point_coord</code>: A single point as "lat lon"</li> + * <li><code>point_coord</code>: A single point as "lat lon"</li> + * <li><code>polygon_coords</code>: A space-separated string of lat/lon pairs …</li>src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java (1)
24-29: Same Javadoc parameter-order mismatch as aboveThe implementation (
closestPointToPolygon) unpacks the first arg as the point and the second as the polygon, but the list documents them in the opposite order. Please swap to avoid confusion.Also, the sample string on line 26/27 is split by an unintended newline (
27 .174884). Consider joining the literal so Javadoc renders correctly.src/test/java/org/javarosa/xpath/test/XPathEvalTest.java (2)
613-619: Redundant duplicate test blockThe two
closest-point-on-polygon('27.176 78.041', …)assertions are identical (lines 613-615 and 617-619). Keeping both adds runtime with no coverage gain.- testEval( - "closest-point-on-polygon('27.176 78.041','27.174957 78.041309 27.174884 78.042574 27.175493 78.042661 27.175569 78.041383')", - null, null, "27.175569 78.041383");
666-687: Potentially unintended longitude/latitude reversal & further duplicates
The input
'27.175 91.040'etc. follow the new lat lon convention, but one case ('91.043 27.176', line 688-690) reverts to lon lat. Please confirm this is deliberate.Several calls are repeated twice with identical expectations (lines 666-675 vs 685-687). Removing the duplicates keeps the suite concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java(2 hunks)src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java(2 hunks)src/test/java/org/javarosa/xpath/test/XPathEvalTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| testEval("is-point-inside-polygon('78.0187201 27.203509','78.0186987 27.2043773 78.0187201 27.203509')", null, null, | ||
| new XPathException( | ||
| "Invalid polygon: Self-intersection")); // Only 2 points, not a polygon | ||
| "closest-point-on-polygon('91.043 27.176','27.174957 91.041309 27.174884 91.042574 27.175493 91.042661 27.175569 91.041383')", |
There was a problem hiding this comment.
how is it working given we are doing lat long now?
There was a problem hiding this comment.
Expected 27.175569 91.041383, got 27.175493 91.042661 when put the right lat lng
Its the case when point is near the edge
There was a problem hiding this comment.
according to the chat gpt the correct answer is "27.17505732 91.04259876" but on deep dive into it i got the reply
(27.175493, 91.042661) → (27.175569, 91.041383) any value range from here is correct depends on the deviations and maths calculation
Product Description
Convert the input type from Lng Lat to Lat Lng format
Added test cases to verify same
Technical Summary
Safety Assurance
Safety story
Automated test coverage
Added test cases to verify the changes
QA Plan
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit
Documentation
Tests