Add generic helpers for parsing ranges and use for years and availabilities#1287
Add generic helpers for parsing ranges and use for years and availabilities#1287alexdewar wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1287 +/- ##
==========================================
+ Coverage 89.46% 89.49% +0.03%
==========================================
Files 57 58 +1
Lines 8361 8389 +28
Branches 8361 8389 +28
==========================================
+ Hits 7480 7508 +28
Misses 581 581
Partials 300 300 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors range-like input parsing into reusable helpers and applies them to year and availability parsing, making availability limits accept a single fixed value and moving year parsing under the input module.
Changes:
- Added generic range parsing helpers and new input-local year parsing.
- Updated availability parsing to use the shared range parser.
- Updated imports after moving year parsing from
src/year.rstosrc/input/year.rs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Enables derive_more support for FromStr. |
src/units.rs |
Adds FromStr support to unit wrapper types. |
src/lib.rs |
Removes the old public year module export. |
src/year.rs |
Removes the old standalone year parser. |
src/input.rs |
Registers range/year input modules and adds sorted-range helper. |
src/input/range.rs |
Adds generic inclusive range parsing helpers. |
src/input/year.rs |
Adds input-scoped year range parsing implementation and tests. |
src/input/process/availability.rs |
Switches availability limit parsing to the shared range parser. |
src/input/process/flow.rs |
Updates year parser import path. |
src/input/process/investment_constraints.rs |
Updates year parser import path. |
src/input/process/parameter.rs |
Updates year parser import path. |
src/input/commodity/levy.rs |
Updates year parser import path. |
src/input/agent/* |
Updates year parser import paths in agent input readers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// If `valid_years` is empty, unsorted or contains duplicates. | ||
| pub fn parse_year_str(s: &str, valid_years: &[u32]) -> Result<Vec<u32>> { | ||
| assert!(!valid_years.is_empty(), "`valid_years` cannot be empty"); |
There was a problem hiding this comment.
I think this is the correct behaviour. valid_years should already have been validated by the caller and, if it hasn't been, then that's a coding bug, so panicking is appropriate.
9c65d45 to
baff99f
Compare
It's not used outside of `input`, so let's keep it in here along with all the other input-related code.
baff99f to
450cc51
Compare
tsmbland
left a comment
There was a problem hiding this comment.
I was never a fan of #1183 for reasons I tried to outline in the issue. If we're doing this then we need to make this SUPER clear in the documentation, otherwise I can almost guarantee that someone will fall into this trap
Description
There are currently two contexts in which we allow users to enter values with a range-like syntax in input files: for years and availabilities. There are also some other places where we may want to use the same syntax in future (e.g., #143, #1106).
There are some small inconsistencies between the way the syntax works with availabilities and years, notably in that you cannot express an availability range where the lower and upper bounds are the same with a single value (i.e. you have to write
0.5..0.5rather than0.5).I think it makes sense to have generic helper functions for this and to use them everywhere we need this syntax, for the sake of consistency and maintainability, so this is what I've done. I've tried to to keep things mostly as they were before, but there are a few behavioural changes:
..) aren't allowed (I thought it was too confusing for users). We could let users write this asallfor all ranges as we do for year ranges, but I haven't done this yet.1990..1999;2010..2019). It's not that I imagine this feature will be widely used, but I don't think it's unclear to write things this way, and it made sense to write the code this way.For year ranges, we need to be able to specify that the default lower and upper bounds are different from the min and max permissible values (which are actually zero and
u32::MAX, so not limits at all), because of #846. We can remove the extra helper if we ever sort #846 (maybe I should let Copilot have a go...).Unrelated change: I also moved
year.rsintosrc/input/, as its functionality is only used insideinputand its submodules.Closes #1183.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks