-
Notifications
You must be signed in to change notification settings - Fork 28
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
[feature] Check coherence of Geometries in the model and fix it #543
Conversation
49cd262
to
a7bef4b
Compare
a7bef4b
to
820256f
Compare
"category": "ObjectNotFound", | ||
"message": "object_type=route, object_id=RERAF: geometry unknown not found" | ||
"category": "OldPropertyValueDoesNotMatch", | ||
"message": "object_type=route, object_id=RERAF, property_name=route_geometry: property_old_value does not match the value found in the data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is now dead code. https://github.com/CanalTP/transit_model/blob/master/src/apply_rules/property_rule.rs#L397
you may remove the code and add a comment that explains why this case should never happen now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is dead code. The geo_id
is given by an end-user when configuring an apply-rules
process. So it is still possible to end up in this code with an incorrect ID I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case in the link above means
an object has a geometry_id and there is no geometries in the collection of geometries with id = geometry_id
but this case never happen now with your change.
If I'm wrong you should write the test for this case
and geo_id
is not given by end-user but is the geometry_id of an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a unit test for testing this case.
In term of public API, we indeed accept a Model
so the case you're mentioning should never happen. But there is also internal API, like crate::apply_rules::property_rules::update_geometry()
, or crate::apply_rules::property_rules::apply_rules()
(which accept a Collections
, not a Model
) that a future developer might use at some point.
a27a334
to
c578cfe
Compare
e5c15f3
to
6b72fcb
Compare
6b72fcb
to
59eb00c
Compare
@Mergifyio refresh |
Command |
No description provided.