MissingCoordinates: 0 is valid for latitude or longitude #201
Conversation
AntoineAugusti
force-pushed
the
stop_coordinates_valid_zero
branch
from
c500e2b
to
d364eda
Compare
AntoineAugusti
force-pushed
the
stop_coordinates_valid_zero
branch
from
d364eda
to
dd49044
Compare
AntoineAugusti
requested review from
Tristramg,
antoine-de and
a team
as code owners
There was a problem hiding this comment.
The fix looks good to me!
I would tweak the tests if possible to explicitly allow "0 on one coordinate only".
Also, I would recommend tweaking the PR documentation:
We now allow a 0 as the latitude OR longitude field.
replaced by:
We now allow a 0 as the latitude "XOR (EXCLUSIVE OR)" longitude field.
| @@ -133,7 +131,7 @@ fn test_missing() { | |||
| .collect(); | |||
|
|
|||
| assert_eq!(1, missing_coord_issue.len()); | |||
| assert_eq!("AMV", missing_coord_issue[0].object_id); | |||
| assert_eq!("null_island", missing_coord_issue[0].object_id); | |||
There was a problem hiding this comment.
❤️ love null island !
I would probably add a test explicitly allowing DIJON/AMV and verifying that they have one zero and not two in their coordinates.
If that is a bit too complicated to write, just a comment on ligne above, since this is also indirectly tested by the fact that missing_cord_issue.len() == 1 at the moment (but this is less explicit, and would not protect us from a regression if someone else removes AMV/DIJON from the test set in the future, unknowingly.
|
And... I missed the auto-merge part 😄 Not a problem for production though. |
|
I updated the PR description to reflect the XOR. I considered writing assertions proving that we have a stop with 0 latitude and another one with a 0 longitude but I'm not proficient enough at writing Rust so this will be for another time or another contributor! |
Thanks!
Gotcha 👍 |
MissingCoordinateswas a bit too strict, and0was rejected for a stop latitude or longitude.We now allow a
0as the latitude XOR (EXCLUSIVE OR) longitude field.Logged #202 to add further checks for stop coordinates.