[feature] Improve line names #747
Conversation
src/model.rs
Outdated
| @@ -1153,6 +1153,31 @@ impl Collections { | |||
| } | |||
| } | |||
|
|
|||
| /// If a line name is empty, it's set with the name of its first "forward" route (in alphabetical order) | |||
There was a problem hiding this comment.
hum maybe it would be better to put this in a separate enhancers module like what has been done in #738 ?
There was a problem hiding this comment.
It’s prettier. Done 👍
src/model.rs
Outdated
| @@ -1153,6 +1153,31 @@ impl Collections { | |||
| } | |||
| } | |||
|
|
|||
| /// If a line name is empty, it's set with the name of its first "forward" route (in alphabetical order) | |||
| pub fn enhance_line_names(&mut self, lines_to_routes: &impl Relation<From = Line, To = Route>) { | |||
| let mut line_names: HashMap<Idx<Line>, String> = HashMap::new(); | |||
There was a problem hiding this comment.
very minor and quite picky but if I understand this correctly, this could be a Vec<(Idx<Line>, String)> no?
There was a problem hiding this comment.
Lighter indeed ! Done
src/model.rs
Outdated
|
|
||
| #[test] | ||
| fn non_empty_line_with_non_empty_route() { | ||
| let mut collections = Collections::default(); |
There was a problem hiding this comment.
If the line were handled by the ModelBuilder you can use it here and I think it would be nice to.
This could be written as
let model = transit_model_builder::ModelBuilder::default()
.line("line_id1", |l| {l.name = "my line id".to_owned();})
.route("route_id1", |r| {
r.line_id = "line_id1".to_owned();
r.direction_type = Some("forward".to_owned());
r.name = "my route id1".to_owned();
})
.build();There was a problem hiding this comment.
I can add it to your PR if you want writing rust would be a nice Jenkins/kube break 😁
There was a problem hiding this comment.
we keep it in mind for later 😄
There was a problem hiding this comment.
I'm wondering why enhance_route_name was not used for that. A function like the following one could have been extracted
fn generate_name(trips: impl Iterator<Item = VehicleJourney>) -> String;
and then used for both enhance_route_name and enhance_line_name. This would make it homogeneous in the way we generate these names.
Note however that what I'm proposing has a substantial performance impact compared to your solution since calculating frequencies is more work and is obviously different in term of requirement.
ArnaudOggy
force-pushed
the
improve_line_names
branch
from
47fd7a6 to
a414c65
Compare
as seen together we will keep it simple for the moment, but indicate this possibility in code comment, and in the associated request ticket |
ArnaudOggy
force-pushed
the
improve_line_names
branch
from
a414c65 to
2fa142c
Compare
| } | ||
| } | ||
| for (line_idx, line_name) in line_names { | ||
| // note: choice is made to keep 'forward_line_name' and 'backward_line_name' as its are |
There was a problem hiding this comment.
| // note: choice is made to keep 'forward_line_name' and 'backward_line_name' as its are | |
| // note: choice is made to keep 'forward_line_name' and 'backward_line_name' as it is |
If a line name is empty, it's set with the name of its first "forward" route (in alphabetical order)
Ref. ND-1229
Note: apparently no fixtures to update for its little brother tartare-tools 😉