Implemented subdivisions for US & DE #16
Conversation
src/data/helper.rs
Outdated
| return; | ||
| } | ||
|
|
||
| let m = national_holidays |
There was a problem hiding this comment.
I'm not sure if it is possible to have a national and a subdivision holiday on the same day, but they could be merged into one with this code instead:
let mut m = national_holidays.remove(&year).unwrap_or_default();
for (date, name) in subdivision_holidays {
let national_holiday_name = m.remove(&date).map(|h| h.name + ", ").unwrap_or_default();
m.insert(
date,
Holiday::new(country, national_holiday_name + county_name, date, name),
);
}There was a problem hiding this comment.
I guess this approach is still using big memory size.
Your approach is merge national holiday + subidivsion holiday at "build" time. My suggestion is to do the merge at runtime, i.e. "get", "iter" or "contains". Could you consider this approach?
There was a problem hiding this comment.
Sorry it took so long. Let me know if you are happy with my implementation
I wasn't sure how to implement this at first, but the code works out pretty well. |
src/data/helper.rs
Outdated
| return; | ||
| } | ||
|
|
||
| let m = national_holidays |
There was a problem hiding this comment.
I guess this approach is still using big memory size.
Your approach is merge national holiday + subidivsion holiday at "build" time. My suggestion is to do the merge at runtime, i.e. "get", "iter" or "contains". Could you consider this approach?
|
|
||
| let mut national_holidays = de::build(years)?; | ||
|
|
||
| build_year(years, 2000, [], &mut map, COUNTY_CODE, COUNTY_NAME); |
There was a problem hiding this comment.
Is this because of no regional public holidays in the year 2000?
| pub fn build(years: Option<&std::ops::Range<Year>>) -> Result<HolidayPerYearMap> { | ||
| let mut map = HashMap::new(); | ||
|
|
||
| let mut national_holidays = de::build(years)?; |
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Can you add a test case where hholidays retrieved from subdivision contains all national holidays?
#9
I implemented subdivisions as their own '
Country' value.Currently I only implemented the subdivisions for US and DE.
I also refactored the code a bit in the process.
I used this command to generate the code:
python3 gen.py; rustfmt src/**/*.rsTODO