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

feat: NIO.TimeAmount(string:) and TimeAmount.description #3046

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

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.

Sources/NIOCore/EventLoop.swift Show resolved Hide resolved
Sources/NIOCore/EventLoop.swift Outdated Show resolved Hide resolved
Sources/NIOCore/EventLoop.swift Show resolved Hide resolved
Sources/NIOCore/EventLoop.swift Outdated Show resolved Hide resolved
@weissi weissi requested a review from glbrntt January 13, 2025 09:31
Sources/NIOCore/EventLoop.swift Outdated Show resolved Hide resolved
Sources/NIOCore/EventLoop.swift Outdated Show resolved Hide resolved
@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.

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