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.

Sign up for GitHub

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

Jump to bottom

unit parsing/pretty printing support for TimeAmount #2504

Open
weissi opened this issue Aug 17, 2023 · 15 comments · May be fixed by #3046
Open

unit parsing/pretty printing support for TimeAmount #2504

weissi opened this issue Aug 17, 2023 · 15 comments · May be fixed by #3046
Labels
kind/enhancement Improvements to existing feature.

Comments

@weissi
Copy link
Member

weissi commented Aug 17, 2023

I frequently need to parse 500 ms into TimeAmount.milliseconds(500) and also render .nanoseconds(17) as 17 ns. So I usually have to write code akin to

extension TimeAmount {
     init(_ userProvidedString: String, defaultUnit: String) throws {
         let string = String(userProvidedString.filter { !$0.isWhitespace }).lowercased()
         let parsedNumbers = string.prefix(while: { $0.isWholeNumber || $0.isPunctuation })
         let parsedUnit = string.dropFirst(parsedNumbers.count)

         guard let numbers = Int64(parsedNumbers) else {
             throw ValidationError("'\(userProvidedString)' cannot be parsed as number and unit")
         }
         let unit = parsedUnit.isEmpty ? defaultUnit : String(parsedUnit)

         switch unit {
         case "h", "hr":
             self = .hours(numbers)
         case "min":
             self = .minutes(numbers)
         case "s":
             self = .seconds(numbers)
         case "ms":
             self = .milliseconds(numbers)
         case "us":
             self = .microseconds(numbers)
         case "ns":
             self = .nanoseconds(numbers)
         default:
             throw ValidationError("Unknown unit '\(unit)' in '\(userProvidedString)")
         }
     }

     var prettyPrint: String {
         let fullNS = self.nanoseconds
         let (fullUS, remUS) = fullNS.quotientAndRemainder(dividingBy: 1_000)
         let (fullMS, remMS) = fullNS.quotientAndRemainder(dividingBy: 1_000_000)
         let (fullS, remS) = fullNS.quotientAndRemainder(dividingBy: 1_000_000_000)

         if remS == 0 {
             return "\(fullS) s"
         } else if remMS == 0 {
             return "\(fullMS) ms"
         } else if remUS == 0 {
             return "\(fullUS) us"
         } else {
             return "\(fullNS) ns"
         }
     }
 }

and I don't think I should have to write this. NIO should have it :).
@weissi
Copy link
Member Author

weissi commented Aug 17, 2023

The pretty printed version should probably just be description.

@weissi
Copy link
Member Author

weissi commented Aug 17, 2023

@Lukasa / @glbrntt could be a good starter bug, no?

@weissi weissi added the kind/enhancement Improvements to existing feature. label Aug 17, 2023
@glbrntt
Copy link
Contributor

glbrntt commented Aug 18, 2023

Seems useful in the long tail so I wouldn't be opposed to this. We just need to agree on what formats we accept when parsing.

DateComponentsFormatter produces:

  • 4 hrs, 5 min, 20 secs (.short)
  • 4hrs 5min 20secs (.abbreviated)
  • 4h 5m 20s (.brief)

I think we should allow an optional space between number and unit and accept:

  • hrs/h
  • min/m
  • secs/s
  • millis/ms
  • micros/us (µs as well?)
  • nanos/ns

@Lukasa
Copy link
Contributor

Lukasa commented Aug 18, 2023

I really don't want us to get into the business of parsing textual time intervals. That way lies madness. I'm fine with us printing in somewhat nice formats, but the slippery feature slope here is very steep. Immediate follow-up questions:

  • why support nanos but not nanoseconds?
  • why support nanoseconds but not nanosecondes?
  • why support nanoseconds but not ナノ秒?
  • why support hours but not days, weeks, fortnights, or lunar cycles?
  • why support nanoseconds but not attoseconds?
  • what do negative numbers mean?

I'm really reluctant to even start down this road.

@weissi
Copy link
Member Author

weissi commented Aug 18, 2023

@glbrntt arbitrary spaces are already allowed

@Lukasa The problem is that everybody then has to repeat this. We can put it into NIO extras but I really think we should have that.

Your concerns are real but that can be solved by documentation where we document what we support. This isn't suggesting to add the one parsing function, it's merely suggesting to add one that you can use if you need specifically this. Happens often to me, unhappy that I have to C&P the code.

@Lukasa
Copy link
Contributor

Lukasa commented Aug 18, 2023

Everyone who speaks English and only wants a specific set of letters.

My problem here is that parsing time intervals from text is a general problem. If that problem is in scope, then it's actually in scope and we should try to solve it, but that does not involve this specific format. Documenting what we support doesn't prevent any of the actual problems, it just invites people to widen our support, unless we have a very clear concrete reason for why this format and only this format are supported in the library.

@weissi
Copy link
Member Author

weissi commented Aug 18, 2023

Everyone who speaks English and only wants a specific set of letters.

My problem here is that parsing time intervals from text is a general problem. If that problem is in scope, then it's actually in scope and we should try to solve it, but that does not involve this specific format. Documenting what we support doesn't prevent any of the actual problems, it just invites people to widen our support, unless we have a very clear concrete reason for why this format and only this format are supported in the library.

Specifically it's not only this format. This would be one format. Just like /usr/bin/units has its random formats (cat /usr/share/misc/units.lib | grep -A15 Time) we can have ours. And ours can be more useful than /usr/bin/units because it only deals in the time domain so doesn't need to deal with ms which could be 'milliseconds', 'metres', 'miles, ...?

@Lukasa
Copy link
Contributor

Lukasa commented Aug 18, 2023

So my question here is why should NIO define a custom format language for time units? Why is this not more properly part of Foundation or the Swift standard library?

@weissi
Copy link
Member Author

weissi commented Aug 18, 2023 •
edited
Loading

So my question here is why should NIO define a custom format language for time units? Why is this not more properly part of Foundation or the Swift standard library?

Because it's for NIO.TimeAmount. Fine by me if we put it into NIOFoundationCompat but I still think that'd be a mistake. It's important to also locale-independently parse this kinda stuff. I use this for command line tools and I really don't want that to be localised or else the arguments now mean different things depending on LANG which would be bad.

@natikgadzhi
Copy link
Contributor

Disclaimer: noob learning question, might be dumb.

So my question here is why should NIO define a custom format language for time units? Why is this not more properly part of Foundation or the Swift standard library?

So what if we made another step to remove the amount of code that covers general concerns in NIO, and replaced NIO.TimeInterval with Swift 5.7's Duriation? I think that once Swift 5.9 is out, we can bump the minimal supported Swift version to 5.7?

If that is viable so far, we could then have a small library with some regular time duration formats outside NIO, in one of the satellite support libraries that SSWG has, similar to apple/swift-http-types?

If that is still viable, then we would probably have to add Duration support, keep TimeInterval support and deprecate the latter. Is it okay then that the formatting funcs would only exist for Duration?

@dnadoba
Copy link
Member

dnadoba commented Sep 3, 2023

Yes, we can and will bump the Swift version and "optional" support isn't a problem even before that. However Swift.Duration is only available on newer Apple platforms (iOS 16.0+, macOS 13.0+) which doesn't allow us to exclusively use them for quite some time. Deprecating TimeInterval is therefore unlikely.

@ktoso
Copy link
Member

ktoso commented Sep 4, 2023

For what it's worth I also keep pasting around code doing such formating into every project we work on tbh. It'd definitely be worth adding in NIO.

I don't agree that it suddenly becomes a localization issue. It's fine to have basic functionality and limit yourself to the "basic" version of it.

Method names should be long "nanoseconds", formatting "ns" or the other short versions. Space or not in parsing either should be fine.

@Austinpayne
Copy link
Contributor

Austinpayne commented Feb 24, 2024 •
edited
Loading

FWIW I've always thought golang had good APIs around duration strings. I too have written some extensions to TimeAmount to get string parsing/descriptions: https://pkg.go.dev/time#ParseDuration and https://pkg.go.dev/time#Duration.String.

@weissi
Copy link
Member Author

weissi commented Dec 15, 2024

Please could we add this? I don't know how many times I've copy & pasted this already :|

@natikgadzhi
Copy link
Contributor

Hey, let me put something up and get back into NIO. I like the suggestion @glbrntt made here on the range of supported formats, that gives me a good starting point, and a list of things write unit tests for.

@natikgadzhi natikgadzhi linked a pull request Jan 12, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: NIO.TimeAmount(string:) and TimeAmount.description natikgadzhi/swift-nio
7 participants
@natikgadzhi @ktoso @weissi @Lukasa @glbrntt @dnadoba @Austinpayne