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

Implement FromStr for Timeout #169

Merged
merged 2 commits into from Dec 13, 2022

Conversation

rnestler
Copy link
Contributor

Useful when wanting to pass it as a command line argument to an application.

I recently needed to pass the notification timeout on the command line. I ended up implementing a custom function, but thought maybe it would be a nice addition:

See https://github.com/rnestler/reboot-arch-btw/pull/106/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR18-R25

If you think this is a good thing, then I'd of course also add tests and whatever else you think is necessary.

@hoodie
Copy link
Owner

hoodie commented Dec 10, 2022

really nice, I love this!
could you document the accepted variants and change the commit message to start with feat: and then a lower case letter?

Thank you

rnestler and others added 2 commits December 11, 2022 11:40
Useful when wanting to pass it as a command line argument to an
application.
@hoodie
Copy link
Owner

hoodie commented Dec 11, 2022

can you please allow me to push onto your branch?

@rnestler
Copy link
Contributor Author

can you please allow me to push onto your branch?

The "Allow edits and access to secrets by maintainers" is checked:
grafik

Isn't this enough to give you push access?

could you document the accepted variants and change the commit message to start with feat: and then a lower case letter?

Sure thing. (Even if I'm curious about the lower case letter start, since it is quite counter to any commit message guideline I know of)

@rnestler rnestler force-pushed the add-form-str-impl-for-timeout branch 2 times, most recently from 284e3df to b6c11dc Compare December 12, 2022 21:17
@rnestler rnestler marked this pull request as ready for review December 12, 2022 21:20
match s {
"default" => Ok(Timeout::Default),
"never" => Ok(Timeout::Never),
milliseconds => Ok(Timeout::Milliseconds(u32::from_str(milliseconds)?)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also allows to create Timeout::Milliseconds(0) by passing "0", but I don't think it is worth to handle the special case, since it is also possible to just construct a Timeout::Milliseconds(0) directly.

@hoodie hoodie force-pushed the add-form-str-impl-for-timeout branch from b6c11dc to 208d55d Compare December 13, 2022 18:45
@hoodie hoodie merged commit e45ddb7 into hoodie:main Dec 13, 2022
@rnestler rnestler deleted the add-form-str-impl-for-timeout branch December 17, 2022 17:32
rnestler added a commit to rnestler/reboot-arch-btw that referenced this pull request Jan 14, 2023
This included the `FromStr` for `Timeout` implementation
(hoodie/notify-rust#169) so we can get rid of
our custom implementation.
rnestler added a commit to rnestler/reboot-arch-btw that referenced this pull request Jan 14, 2023
This included the `FromStr` for `Timeout` implementation
(hoodie/notify-rust#169) so we can get rid of
our custom implementation.
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.

None yet

2 participants