-
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] handle route comments on line when using read_as_line #741
Conversation
0287c36
to
7593852
Compare
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'm a bit annoyed that generate_route_comment()
might return a Comment
either with an identifier prefixed with route:
or line:
. Then only after in read_routes()
, the code is branching on read_as_line
and decide to use this comment in a line or in a route. So in terms of code architecture, it's possible to add a comment route:
in a line
which would be wrong. I know both actions are conditioned to read_as_line
so it won't happen at the moment but since the logic is in 2 different places, future maintenance of the code might break this invariant. I'm wondering how the code could be refactored to better reflect this.
I'm not sure if I'm very clear about what I'm trying to explain, so do not hesitate to ping me in private to better explain.
src/gtfs/read.rs
Outdated
route.desc.as_ref().map(|desc| objects::Comment { | ||
id: "route:".to_string() + &route.id, | ||
id: prefix.to_string() + &route.id, |
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.
id: prefix.to_string() + &route.id, | |
id: format!("{}:{}", prefix, route.id), |
And maybe remove the :
from the prefix
variable.
I think I understand what annoys you, but I’m not sure how to adress it. First, we can rename it |
I propose this PR that might address the problem. Tell me what you think about it. |
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.
Good for me. Do not hesitate to squash/rebase if needed.
I just squashed you changes. Thank you! |
cf. ND-1075 and replaces #736