Skip to content

Conversation

@natikgadzhi
Copy link
Contributor

Adds TimeAmount.init(string:) and TimeAmount.description for parsing time amounts from strings and pretty printing them.

Closes #2504.

Motivation:

Had a minute, wanted to work on Swift-NIO a bit more, and saw @weissi still wanted this.

Modifications:

It's largely based on the snippet @weissi made in #2504 with a few changes:

  • Added unit tests.
  • Added support for more unit aliases for convenience based on @glbrntt's suggestion.
  • Changed .gitignore to ignore .build everywhere, I've got a few of them when testing locally.

Open questions:

  • I originally thought perhaps I should add support for multiple number and unit pairs, i.e. 1h 31m, but decided against it. Feels like an edge case. Happy to add this if you think it's needed.

Copy link
Contributor Author

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Code walkthrough comments to kick things off.

extension TimeAmount: CustomStringConvertible {

/// Errors thrown when parsint a TimeAmount from a string
public enum ValidationError: Error, Equatable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's a great idea to introduce a new public error type. Can I get away with re-using something?

Copy link
Member

Choose a reason for hiding this comment

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

I doubt that anybody would really catch that error, so we could also just make this an internal error type. That way you can still throw & print it but NIO wouldn't be forced into the API.

@weissi weissi requested a review from glbrntt January 13, 2025 09:31
@natikgadzhi
Copy link
Contributor Author

Alright, cleaned this up. Thank you for taking a look at this quickly, and agreeing to have this as part of NIO, @Lukasa!

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jan 14, 2025
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, thanks!

/// - defaultUnit: Unit to use if no unit is specified in the string
///
/// - Throws: ValidationError if the string cannot be parsed
public init(_ userProvidedString: String, defaultUnit: String = "s") throws {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I wouldn't default defaultUnit to anything. Why is it seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this honestly makes sense AND this is not covered in tests, one sec.

@Lukasa Lukasa enabled auto-merge (squash) January 14, 2025 14:59
auto-merge was automatically disabled January 14, 2025 17:16

Head branch was pushed to by a user without write access

@natikgadzhi
Copy link
Contributor Author

@Lukasa @weissi I cleaned this up, looking into what I'm doing wrong with formatting, and picking up how to run soundness checks locally so I don't waste CI time more.

@Lukasa Lukasa merged commit f5773c1 into apple:main Jan 16, 2025
34 of 35 checks passed
@natikgadzhi
Copy link
Contributor Author

Thank you for helping me though this folks 🖤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unit parsing/pretty printing support for TimeAmount

3 participants