Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic issue (#80) that occurred when encountering invalid opening hours expressions (e.g., "sunset") during data collection from Overture Maps. The fix introduces a new DuringExpression enum that can handle both valid OSM opening hours expressions and unexpected string values.
Changes:
- Introduced
DuringExpressionenum to gracefully handle invalid opening hours expressions asUnexpected(String)variant - Moved binary entry point from
src/bin/bambam_omf.rstosrc/main.rsfollowing project conventions - Exposed
batch_sizeandobject_storeas CLI arguments for better configurability - Refactored latest release detection to use
max_by_keyinstead ofsort+pop
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam-omf/src/main.rs | New main entry point with proper error handling via Result return type |
| rust/bambam-omf/src/collection/record/during_expression.rs | New enum type to handle both valid and invalid opening hours expressions |
| rust/bambam-omf/src/collection/record/transportation_segment.rs | Updated to use DuringExpression instead of OpeningHoursExpression; contains commented-out code |
| rust/bambam-omf/src/collection/record/mod.rs | Added during_expression module |
| rust/bambam-omf/src/collection/object_source.rs | Added ValueEnum derive, Display impl, and serde rename attributes for CLI support |
| rust/bambam-omf/src/collection/collector.rs | Refactored to use max_by_key and improved error message |
| rust/bambam-omf/src/app/omf_app.rs | Added batch_size and object_store CLI arguments |
| rust/bambam-omf/src/app/network.rs | Updated to accept and use new CLI parameters |
| rust/bambam-omf/src/bin/bambam_omf.rs | Removed - functionality moved to main.rs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/bambam-omf/src/collection/record/transportation_segment.rs
Outdated
Show resolved
Hide resolved
rust/bambam-omf/src/collection/record/transportation_segment.rs
Outdated
Show resolved
Hide resolved
yamilbknsu
left a comment
There was a problem hiding this comment.
Looks good, thank you for picking up the organization of the input parameters along the way!
I have move a couple stuff around in this fashion in my branches but nothing too complex to solve I think.
Also, about the timeouts in #80, I think those are the same ones I was randomly getting last year and I thought I solved them with the io_handle thing. If that is not enough, I believe the row group optimization should totally do it, but we can keep an eye on that.
…ize, object_store params
|
i rolled back this change because of conflicts with other concurrent work: |
this PR closes #80 by making the "during" field into an enum that can either be an OSM
OpeningHoursExpressionor just a String in the case that the opening hours is not valid.while attempting to download denver, i encountered a segment with during = "sunset", which is not a valid entry. it will now be captured in the
Unexpected(String)variant of the newDuringExpression.along the way,
sortandpopwithmaxwhile finding the latest release