ND-163 - Enrich NTFS with HelloGo fares #267
Conversation
| } | ||
|
|
||
| let line_ref = get_line_ref(fare_frame)?; | ||
| let line_id = get_line_id_from_line_ref(service_frame, line_ref)?; |
There was a problem hiding this comment.
As a reminder, do not forget to check if the line exists in collections.lines.
There was a problem hiding this comment.
Thanks for the reminder. It is now done in the commit that introduce modifications to handle prefixes (see 09154d7).
woshilapin
force-pushed
the
ND-163
branch
2 times, most recently
from
09154d7
to
04bd8c8
Compare
src/hellogo_fares/read.rs
Outdated
|
|
||
| fn load_fare_file(collections: &mut Collections, root: &Element) -> Result<()> { | ||
| let prefix = get_prefix(&collections)?; | ||
| let prefix_column = prefix.clone() + ":"; |
There was a problem hiding this comment.
Only concatenate if a prefix exists, otherwise the result of the variable does not make sense (and objects in the collection will not be found).
There was a problem hiding this comment.
Do you mean something like this.
| let prefix_column = prefix.clone() + ":"; | |
| let prefix_column = if prefix.is_empty() { | |
| "".to_string() | |
| } else { | |
| prefix.clone() + ":" | |
| }; |
There was a problem hiding this comment.
Oh, I actually made the get_prefix() function used just above return a Result, meaning, it would fail if no prefix is found. Maybe I need to convert to an Option. Let me try to rework all of that.
Another question that came up is, what if the contributor_id is equal to NTM: (very edgy case, I agree). Should I detect the prefix NTM (but that would essentially mean the ID is empty), or should I consider that it is not prefixed?
There was a problem hiding this comment.
So I discussed the edge cases with @prhod and:
- If
contributor_idisNTM:(so without a suffix), just act as normal and extract the prefix - If
contributor_idis something like123(no prefix), process the NTFS as a non-prefixed NTFS
I also realized that the name prefix_column didn't make much sense because... what I meant was prefix_with_colon. I also did the change accordingly. Look commit 599ab46 for the changes.
woshilapin
force-pushed
the
ND-163
branch
4 times, most recently
from
9a96037
to
a5c56c7
Compare
src/hellogo_fares/read.rs
Outdated
| Ok(origin_destinations) | ||
| } | ||
|
|
||
| fn load_fare_file(collections: &mut Collections, root: &Element) -> Result<()> { |
There was a problem hiding this comment.
I don't like load_fare_file as I don't see files but I don't have a better name
There was a problem hiding this comment.
You are right, it's not a file. I changed it to load_netex_fares which might not be perfect either but maybe a little bit better (see ee4c4b8).
src/hellogo_fares/read.rs
Outdated
| /// For HelloGo fares connector, we need the prefix of the input NTFS. | ||
| /// The prefix will be extracted from the 'contributor_id' | ||
| fn get_prefix(collections: &Collections) -> Option<String> { | ||
| if let Some((_, contributor)) = collections.contributors.iter().next() { |
There was a problem hiding this comment.
collections
.contributors
.values()
.next()
.and_then(|c| c.id.find(':').map(|index| c.id[..index].to_string()))There was a problem hiding this comment.
Nice rustic improvement, done in 0c57c90.
src/hellogo_fares/read.rs
Outdated
| } | ||
|
|
||
| fn load_fare_file(collections: &mut Collections, root: &Element) -> Result<()> { | ||
| let prefix_with_colon = if let Some(prefix) = get_prefix(&collections) { |
There was a problem hiding this comment.
let prefix_with_colon = get_prefix(&collections)
.map(|prefix| prefix + ":")
.unwrap_or_else(|| "".to_string());There was a problem hiding this comment.
Nice rustic improvement, done in 5a3d82b.
src/hellogo_fares/utils.rs
Outdated
| key, | ||
| element.name() | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
Done in 812eec1.
src/hellogo_fares/utils.rs
Outdated
| .try_only_child("FrameDefaults")? | ||
| .try_only_child("DefaultCurrency")? | ||
| .text(); | ||
| if iso4217::alpha3(currency.as_str()).is_some() { |
There was a problem hiding this comment.
or early return, as you want
if iso4217::alpha3(currency.as_str()).is_none() {
bail!("Failed to validate '{}' as a currency", currency);
}
Ok(currency)There was a problem hiding this comment.
Done in 3ccab53.
src/hellogo_fares/read.rs
Outdated
|
|
||
| impl From<String> for TicketUse { | ||
| fn from(ticket_id: String) -> Self { | ||
| let ticket_use_id = format!("TU:{}", ticket_id.clone()).to_string(); |
There was a problem hiding this comment.
let ticket_use_id = format!("TU:{}", &ticket_id);
There was a problem hiding this comment.
I choose to do it without the format!() macro. See 0c7b22f.
|
I also added some more integration tests for NTFS without prefixes. See 22d130d. |
woshilapin
force-pushed
the
ND-163
branch
2 times, most recently
from
88e0612
to
df1cc03
Compare
No description provided.