waltzofpearls / dateparser

Parse dates in commonly used string formats with Rust.
MIT License
41 stars 8 forks source link

Fix parse_with() #33

Closed kdwarn closed 1 year ago

kdwarn commented 1 year ago

I have been using the dateparser library in developing a project of mine (https://codeberg.org/kdwarn/ts-cli) and found a bug with the parse_with function. As detailed in an issue there (https://codeberg.org/kdwarn/ts-cli/issues/18), it wasn't converting correctly to UTC. Essentially, the default_time parameter to Parse::new needs to be different if using parse and parse_with_timezone or if using parse_with. The solution I came up with was to change Parse struct's field default_time to an Option<NaiveTime> rather than a NaiveTime, and then in the various functions either use the provided default_time if given or create one at that time. Given this, I imagine you'll want to do a version bump.

In the existing code, there were only doctests that covered parse_with (no separate unit tests), and at least one was failing. I wrote out what I think to be a fairly comprehensive set of units tests, and both they and the doctests are now passing. I had to modify a lot of tests to account for changing default_time from NaiveTime to Option<NativeTime>.

Note that I also bumped to the new 2021 edition of Rust, and there seems to be no issues with that.

Also, I explicitly bumped to 0.4.24 of chrono, and then turned off deprecation warnings, as there are many. I figure they can be addressed separately after this.

waltzofpearls commented 1 year ago

Hey @kdwarn, thanks for the PR! I will have a look when I get home.

kdwarn commented 1 year ago

To be clear, dateparser's tests are passing. I did not look at belt itself.

kdwarn commented 1 year ago

Ok, clippy shouldn’t complain anymore.

If you want, I can squash this all into one commit.

waltzofpearls commented 1 year ago

@kdwarn Thanks for fixing the linter warnings. I will squash the commits when merging the PR. I had a look at your PR. Looks good to me. I'm working on fixing belt tests in a separate branch.

kdwarn commented 1 year ago

Excellent!