-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Advance POT-Creation-Date by 1 minute on normalization #212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this looks great! I just think we should avoid erroring out in case the date cannot be parsed for some unknown reason.
i18n-helpers/src/normalize.rs
Outdated
if let Ok(pot_creation_date) = dateparser::parse(&new_catalog.metadata.pot_creation_date) { | ||
new_catalog.metadata.pot_creation_date = pot_creation_date | ||
.checked_add_signed(Duration::minutes(1)) | ||
.ok_or(anyhow::Error::msg("could not advance POT-Creation-Date"))? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you instead leave the date unchanged if it cannot be parsed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -566,4 +576,12 @@ mod tests { | |||
], | |||
); | |||
} | |||
|
|||
#[test] | |||
fn test_normalize_pot_creation_date() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for the case where the date cannot be parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Nice, I had not noticed that 😄 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
==========================================
- Coverage 86.99% 84.82% -2.17%
==========================================
Files 15 15
Lines 3229 3341 +112
Branches 3229 3341 +112
==========================================
+ Hits 2809 2834 +25
- Misses 327 413 +86
- Partials 93 94 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great!
@mgeisler Fixed the linting issues, mind giving approval for the workflow? |
Fixes #117.
Advances POT-Creation-Date by 1 minute rather than 1 second as @mgeisler suggested as it seems like the date format does not include seconds for some reason: https://www.gnu.org/software/trans-coord/manual/gnun/html_node/PO-Header.html