Skip to content
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

add datetime parser #12

Merged
merged 6 commits into from
Jun 7, 2023
Merged

add datetime parser #12

merged 6 commits into from
Jun 7, 2023

Conversation

Benjscho
Copy link
Contributor

@Benjscho Benjscho commented Jun 3, 2023

Moving PR from main coreutils to this crate, after discussion here: uutils/coreutils#4917

Add a relaxed datetime parser. This datetime parser functions by using chronos own parsing utilities and a try/succeed approach to parsing.

This implementation of the datetime parser has some drawbacks and some positives. On the positive side:

  • it was easy to implement
  • it is easy to add more datetime formats to

In order to add additionally supported formats, a developer can add the required format string to the format mod in parse_datetime.rs, and then add it as a potential format to the relevant fmts vec.

On the negative:

  • It is not easily customiseable beyond the supported chrono parsing formats. E.g., chrono does not currently support parsing offsets without trailing zeros. from_str("UTC+1") should return a valid response but chrono fails to parse this.
  • Because it is an attempt driven parser, it is likely not that performant. I have not done any performance testing as part of this change, but I would expect a custom parser to perform much better.

Remaining todo:

  • Provide a combined parser that loosely parses either duration, or datetime, or permutations of datetime and duration.

Add a relaxed datetime parser. This datetime parser functions by using `chrono`s
own parsing utilities and a try/succeed approach to parsing.

This implementation of the datetime parser has some drawbacks and some
positives. On the positive side:
- it was easy to implement
- it is easy to add more datetime formats to

In order to add additionally supported formats, a developer can add the
required format string to the `format` mod in `parse_datetime.rs`, and
then add it as a potential format to the relevant `fmts` vec.

On the negative:
- It is not easily customiseable beyond the supported `chrono` parsing
  formats. E.g., `chrono` does not currently support parsing offsets
  without trailing zeros. `from_str("UTC+1")` should return a valid response
  but `chrono` fails to parse this.
- Because it is an attempt driven parser, it is likely not that
  performant. I have not done any performance testing as part of this
change, but I would expect a custom parser to perform much better.
src/parse_datetime.rs Outdated Show resolved Hide resolved
src/parse_datetime.rs Outdated Show resolved Hide resolved
Co-authored-by: Sylvestre Ledru <[email protected]>
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #12 (7ee33d1) into main (e5d7fbc) will increase coverage by 23.09%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##             main      #12       +/-   ##
===========================================
+ Coverage   41.87%   64.97%   +23.09%     
===========================================
  Files           2        3        +1     
  Lines         511      454       -57     
  Branches      100       93        -7     
===========================================
+ Hits          214      295       +81     
+ Misses        259      111      -148     
- Partials       38       48       +10     
Flag Coverage Δ
macos_latest ?
ubuntu_latest 64.97% <80.00%> (+23.09%) ⬆️
windows_latest 67.97% <80.00%> (+25.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lib.rs 51.16% <ø> (+20.79%) ⬆️
src/parse_datetime.rs 80.00% <80.00%> (ø)

@sylvestre
Copy link
Contributor

looks good. could you please update the readme too ?

Update the README and add a test module to parse_datetime for any
examples presented in the README.
@Benjscho
Copy link
Contributor Author

Benjscho commented Jun 6, 2023

looks good. could you please update the readme too ?

Just updated!

@sylvestre sylvestre merged commit e503de7 into uutils:main Jun 7, 2023
mvo5 added a commit to mvo5/coreutils that referenced this pull request Mar 30, 2024
This commit is a trivial followup for:
uutils#4917
and
uutils/parse_datetime#12

The functionality to parse the datetime was moved into the parse_datetime
crate and the only (tiny) piece left is to call it from `date`.

It also adds the two tests from the original PR#4917.

Closes: uutils#4657

Thanks to Ben Schofield
mvo5 added a commit to mvo5/coreutils that referenced this pull request Mar 30, 2024
This commit is a trivial followup for:
uutils#4917
and
uutils/parse_datetime#12

The functionality to parse the datetime was moved into the parse_datetime
crate and the only (tiny) piece left is to call it from `date`.

It also adds the test-case from the original issue. I did not include
the two tests from PR#4917 because they appear to work even without
this change. I am happy to include them of course if prefered.

Closes: uutils#4657

Thanks to Ben Schofield
cakebaker pushed a commit to uutils/coreutils that referenced this pull request Mar 30, 2024
* date: fix `date -f dates.txt is failing`

This commit is a trivial followup for:
#4917
and
uutils/parse_datetime#12

The functionality to parse the datetime was moved into the parse_datetime
crate and the only (tiny) piece left is to call it from `date`.

It also adds the test-case from the original issue. I did not include
the two tests from PR#4917 because they appear to work even without
this change. I am happy to include them of course if prefered.

Closes: #4657

Thanks to Ben Schofield

* tests: tweak changes to test_date.rs to be more idiomatic

Co-authored-by: Sylvestre Ledru <[email protected]>

---------

Co-authored-by: Sylvestre Ledru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants