[feature] warning if more than one timezone is reported on agency file #705
Conversation
src/gtfs/read.rs
Outdated
| for (current_agency, next_agency) in gtfs_agencies.iter().zip(gtfs_agencies.iter().skip(1)) { | ||
| if current_agency.timezone != next_agency.timezone { | ||
| warn!( | ||
| "different agency timezone: {} - {}", | ||
| current_agency.timezone, next_agency.timezone | ||
| ); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
How about something like this?
| for (current_agency, next_agency) in gtfs_agencies.iter().zip(gtfs_agencies.iter().skip(1)) { | |
| if current_agency.timezone != next_agency.timezone { | |
| warn!( | |
| "different agency timezone: {} - {}", | |
| current_agency.timezone, next_agency.timezone | |
| ); | |
| break; | |
| } | |
| } | |
| let timezones: HashSet<_> = gtfs_agencies.iter().map(|agency| agency.timezone).collect(); | |
| if timezones.len() > 1 { | |
| if current_agency.timezone != next_agency.timezone { | |
| warn!( | |
| "multiple agency timezones found: {:?}", | |
| timezones, | |
| ); | |
| break; | |
| } | |
| } |
src/gtfs/read.rs
Outdated
| testing_logger::validate(|captured_logs| { | ||
| assert_eq!(captured_logs.len(), 2); | ||
| assert_eq!( | ||
| captured_logs[1].body, | ||
| "different agency timezone: Europe/London - Europe/Paris" | ||
| ); | ||
| assert_eq!(captured_logs[1].level, LogLevel::Warn); | ||
| }); |
src/gtfs/read.rs
Outdated
| @@ -576,6 +576,20 @@ where | |||
| { | |||
| let filename = "agency.txt"; | |||
| let gtfs_agencies = read_objects::<_, Agency>(file_handler, filename)?; | |||
|
|
|||
| for (current_agency, next_agency) in gtfs_agencies.iter().zip(gtfs_agencies.iter().skip(1)) { | |||
There was a problem hiding this comment.
Why not just compare all others to the first one (that would be the only referent, and it would save one iterator) ?
It looks weird to me to use a different "referent" each time we compare (in the current version), but it works to output desired warning.
Do we want to point out the less-represented timezone ? Not sure it's super useful, and it would require more code.
There was a problem hiding this comment.
@pbougue
It breaks at the first difference, and without memory allocation 🤔
There was a problem hiding this comment.
To clarify my comment after discussion, I suggest comparing only gtfs_agencies.first() with all the following.
Something along that (not tested at all, maybe store only referent timezone, not agency):
let referent_agency = gtfs_agencies.first();
for agency in gtfs_agencies.iter().skip(1) {
if referent_agency.timezone != agency.timezone {
[...]
}
}Anyway it's just a suggestion not a veto as current code will warn if and only if problems arise.
And indeed I didn't see the break (proposed solution could warn on all differences).
There was a problem hiding this comment.
the number of iterations will be the same.
If we have a, b, ,c and d
my proposal will give :
a compare with b
b compare with c
c compare with d
your proposal will give :
a compare with b
a compare with c
a compare with d
There was a problem hiding this comment.
I like @pbougue's solution because:
- it's one iterator instead of 2 iterators (1N complexity instead of 2N complexity)
- it feels easier to understand what we're trying to do (the
.zip()feel a little less natural)
But in any case, I'm fine with whatever we end up merging.
There was a problem hiding this comment.
I capitulate because it is more readable
patochectp
force-pushed
the
different_agency_timezone
branch
from
b79b3fb to
c0f7eb7
Compare
ref: ND-1005.