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 DurationArgument for parsing java.time.Duration #330

Merged
merged 12 commits into from
Jan 5, 2022

Conversation

tadhgboyle
Copy link
Contributor

@tadhgboyle tadhgboyle commented Dec 21, 2021

Expected syntax: 1d12h4m47s (1 day, 12 hours, 4 minutes, 47 seconds)
Tests are included + passing

Example tab completion:

Screen.Recording.2021-12-21.at.3.15.45.PM.mov

Copy link
Contributor

@FrankHeijden FrankHeijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some things to make gradle happy

tadhgboyle and others added 3 commits December 22, 2021 10:40
…dard/DurationArgumentSuggestionsTest.java

Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
…dard/DurationArgumentTest.java

Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
…dard/DurationArgument.java

Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
@solonovamax
Copy link
Contributor

In the video, you typed in 1d12h88m1s and it accepted this. (At least, in the tab completion)

But, since you cannot have 88 minutes in an hour, should it accept this?
Personally, I don't believe it should accept the input 1d12h88m1s. But, I can very much see an argument for 88m1s, so long as there is nothing in front of the 88 minutes.

Thoughts?

@AlessioGr
Copy link

AlessioGr commented Dec 31, 2021

88m1s would make more sense than 1d12h88m1s, I agree. But, as long as it's able to parse it, why limit it?

Here's a possible scenario where it could be used that way:

Maybe I'd type 1h55m and before I press enter, I realize "oh, maybe 10 minutes more would be better", so I'd quickly change it to 1h65m just for the convenience of having to edit less. Or, same scenario if I write something like 80m and then I realize I want to add another day to it

@FrankHeijden
Copy link
Contributor

Am also in favour of keeping this behaviour, I can see someone using something like 100s ^^

@solonovamax
Copy link
Contributor

Hmmm, fair point.
I can definitely see the usecase of that.

@solonovamax
Copy link
Contributor

Also, here's a few features that should be considered. Whether or not they're added is up to you, I guess.

  • Support for more duration formats:
    • The ISO 8601 duration format (This is ore
      • eg. PT4H57M35S, which is 4 hours, 57 minutes, and 35 seconds.
    • The following colon separated formats:
      • HH:mm:ss.SSS (Note: HH -> hours, mm -> minutes, ss -> seconds, SSS -> milliseconds)
        • Also, the dd:HH:mm:ss.SSS format, which includes days.
    • What about perhaps also supporting a unix timestamp?
      • eg,example if I input the value 628927, it would interpret it as 7 days, 6 hours, 42 minutes, and 7 seconds:
        image
        Or: 511352025 would be interpreted as 5918 days, 10 hours, 13 minutes, and 45 seconds.
        image
  • Currently, it parses "incorrect" formats such as: 1m7s21d (1 minute, 7 seconds, 21 days)
    • The d, h, m, and s elements should be positional. That way, a d always has to be the first element, an m must always either be first or come after an h, etc.
  • What about support for milliseconds and nanoseconds? This was touched on slightly in the "more formats" bit, but adding ms and ns would be very nice.

@tadhgboyle
Copy link
Contributor Author

tadhgboyle commented Dec 31, 2021

Also, here's a few features that should be considered. Whether or not they're added is up to you, I guess.

* Support for more duration formats:
  
  * The [ISO 8601 duration format](https://en.wikipedia.org/wiki/ISO_8601#Durations) (This is ore
    
    * eg. `PT4H57M35S`, which is 4 hours, 57 minutes, and 35 seconds.
  * The following colon separated formats:
    
    * `HH:mm:ss.SSS` (Note: `HH` -> hours, `mm` -> minutes, `ss` -> seconds, `SSS` -> milliseconds)
      
      * Also, the `dd:HH:mm:ss.SSS` format, which includes days.
  * What about perhaps also supporting a unix timestamp?
    
    * eg,example if I input the value `628927`, it would interpret it as 7 days, 6 hours, 42 minutes, and 7 seconds:
      ![image](https://user-images.githubusercontent.com/46940694/147838050-368e8d6c-a117-41a0-a8c8-08b519619320.png)
      Or: `511352025` would be interpreted as 5918 days, 10 hours, 13 minutes, and 45 seconds.
      ![image](https://user-images.githubusercontent.com/46940694/147838075-7fa1e28f-d4b5-4d2b-b2ef-daf785d168a0.png)

* Currently, it parses "incorrect" formats such as: `1m7s21d` (1 minute, 7 seconds, 21 days)
  
  * The `d`, `h`, `m`, and `s` elements should be positional. That way, a `d` always has to be the first element, an `m` must always either be first or come after an `h`, etc.

* What about support for milliseconds and nanoseconds? This was touched on slightly in the "more formats" bit, but adding `ms` and `ns` would be very nice.

Thank you for the detailed comment!

  • I am open to enforcing positional d/h/m/s if that is wanted by more people - but for now I will leave it as is, since there isnt much of a downside.
  • Adding support for parsing seconds into d/h/m/s is interesting, but I don't think I would add this since the whole purpose of this argument is to make duration's human friendly to write.
  • ISO parsing is probably not something I'd want to do. Better to keep it with only 1 way to enter the duration as to not confuse users.
  • Colon separated i'm gonna say no to since yet another way to enter a duration into this parser is going to complicate tab completion, parsing, and the user experience.
  • Genuinely curious what a practical use case for ms and ns support is?

@solonovamax
Copy link
Contributor

At least for ms, I could see some usecase where someone might want greater precision.

Eg. a command that starts a task which runs every 500 ms (0.5s)
Nanoseconds is something that's definitely much harder to find a use for, but while you're adding ms might as well add ns.

Also, I, personally, would say that colon separated times are more important that ISO dates. Because the ISO dates aren't as friendly to humans as colon separated dates

@ParadauxIO
Copy link

ParadauxIO commented Jan 4, 2022

This parser being able to parse "incorrect" timeperiods shouldn't be an issue, I don't understand why the parser needs to care if you

  • Exceed the natural limit of a unit (i.e 61m)
  • Place them in the incorrect mangnitudal order (h, m, s, etc)

These are artificial limitations that don't really provide any value by enforcing them, as you will then need to be able to provide feedback to the user that this was an abitrarily restricted case. Especially for the single-token cases like 100s and such.

I don't see the utility in preventing 1w100m as being a valid construction, as semenaticly it is identical to 1w1h40m. Ensuring the proper token order is also unnecessary, saying 10m1w is equivalent to 1w10m sure it might be unnatural in English to say the former, but that doesn't mean it's unnatural to other speakers, the order in which the units are placed should be arbitrary as they are currently.

I would keep the parser as is. I don't think the ISO 8601 is appropriate for use here, as that will just be needlessly confusing for end-users in my opinion.

@solonovamax
Copy link
Contributor

solonovamax commented Jan 4, 2022

@ParadauxIO
This parser being able to parse "incorrect" timeperiods shouldn't be an issue, I don't understand why the parser needs to care if you

  • Exceed the natural limit of a unit (i.e 61m)
  • Place them in the incorrect mangnitudal order (h, m, s, etc)

These are artificial limitations that don't really provide any value by enforcing them, as you will then need to be able to provide feedback to the user that this was an abitrarily restricted case. Especially for the single-token cases like 100s and such.

Hmm, very fair point. I just felt that inputs like 1s20h1w3m shouldn't be allowed.

But, for single token cases like 100s, it should definitely be allowed.

I'll leave it up to OP to decide if they want to do this.

@ParadauxIO
I would keep the parser as is. I don't think the ISO 8601 is appropriate for use here, as that will just be needlessly confusing for end-users in my opinion.

Also, I'd agree ISO 8601 very much could be needlessly complex.
But, I still think that the dd:hh:mm:ss.SSS format should be considered, and milliseconds/nanoseconds should be supported for greater precision

@Citymonstret
Copy link
Member

Citymonstret commented Jan 4, 2022

Hmm, very fair point. I just felt that inputs like 1s20h1w3m shouldn't be allowed.

We don't want overly opinionated parsers in the core library anyway. Someone could make a future addition to the parser that allows you to toggle stricter behaviour on and off (like the boolean parser's liberal mode), but this should not be the default.

@zml2008 zml2008 enabled auto-merge (squash) January 4, 2022 23:18
@zml2008 zml2008 disabled auto-merge January 4, 2022 23:18
@zml2008 zml2008 changed the base branch from master to 1.7.0-dev January 4, 2022 23:19
@zml2008 zml2008 added enhancement New feature or request parser labels Jan 5, 2022
@zml2008 zml2008 added this to the 1.7.0 milestone Jan 5, 2022
@zml2008 zml2008 merged commit 4dbcb07 into Incendo:1.7.0-dev Jan 5, 2022
@zml2008 zml2008 self-assigned this Jan 5, 2022
final String input = inputQueue.peek();
if (input == null) {
return ArgumentParseResult.failure(new NoInputProvidedException(
DurationArgument.DurationParseException.class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be DurationParser.class?

jpenilla pushed a commit that referenced this pull request Feb 28, 2022
Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
Citymonstret pushed a commit that referenced this pull request Jun 3, 2022
Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
jpenilla pushed a commit that referenced this pull request Jun 19, 2022
Co-authored-by: Frank van der Heijden <frank.boekanier@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants