Fix parse_with() #33
Merged
Fix parse_with() #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ymd_z() for parse_with still needs to be fixed - tests with it are failing and those are currently commented out. Ignore deprecation warnings for now - come back to that after finishing resolving issue ymd_z (as separate pull request).
|
Hey @kdwarn, thanks for the PR! I will have a look when I get home. |
|
To be clear, |
|
Ok, clippy shouldn’t complain anymore. If you want, I can squash this all into one commit. |
|
@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 |
|
Excellent! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been using the
dateparserlibrary in developing a project of mine (https://codeberg.org/kdwarn/ts-cli) and found a bug with theparse_withfunction. As detailed in an issue there (https://codeberg.org/kdwarn/ts-cli/issues/18), it wasn't converting correctly to UTC. Essentially, thedefault_timeparameter toParse::newneeds to be different if usingparseandparse_with_timezoneor if usingparse_with. The solution I came up with was to changeParsestruct's fielddefault_timeto anOption<NaiveTime>rather than aNaiveTime, and then in the various functions either use the provideddefault_timeif 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 changingdefault_timefromNaiveTimetoOption<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.