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 const fn none() #27

Merged
merged 2 commits into from
Jun 30, 2022
Merged

Add const fn none() #27

merged 2 commits into from
Jun 30, 2022

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented May 16, 2022

Both Duration and Instant are explicitly optional types, so why not add this?

Alternative: add NONE as an associated constant. Simpler, but less flexible in case the internal representation changes.

Workaround: both support From<Option<..>> already.

@dhardy
Copy link
Contributor Author

dhardy commented May 16, 2022

Related questions:

  • Should Instant::default() return a "none" value? This is the only sensible value it could return; whether it should depends on the point of view: is "none" an error state or is the type explicitly optional?
  • Should Duration::default() return "none" instead of "zero"?
  • (Is there even a good reason to have your own Duration type here instead of just returning Option<Duration> from Instant::duration_since?)

My motivation: what I actually want is to represent the last time something happened, if any. Option<Instant> works, but is clunky enough that I was going to write my own wrapper, before discovering you basically did that. Panic-free operations is just a bonus from my point of view.

@taiki-e
Copy link
Owner

taiki-e commented May 16, 2022

Thanks! Seems reasonable to have such a feature.

Alternative: add NONE as an associated constant.

Given that these types already have some associated constants, associated constants may be more consistent. That said, I have no strong opinion on this.

Simpler, but less flexible in case the internal representation changes.

It is interesting that associated constants are less flexible than const fns, but I'm not too worried about this as we can define an associated constant using a private const fn.

  • Should Instant::default() return a "none" value? This is the only sensible value it could return; whether it should depends on the point of view: is "none" an error state or is the type explicitly optional?
  • Should Duration::default() return "none" instead of "zero"?

The first purpose of this crate was to use None as an error state, so I think using None as some form of default can be a pitfall for those who use this crate for that purpose.

  • (Is there even a good reason to have your own Duration type here instead of just returning Option<Duration> from Instant::duration_since?)

Sorry, I'm not sure what you mean. The current easytime::Instant::duration_since's signature is equivalent to std.

@dhardy
Copy link
Contributor Author

dhardy commented May 16, 2022

The first purpose of this crate was to use None as an error state, so I think using None as some form of default can be a pitfall for those who use this crate for that purpose.

Both types have an is_none() method, not an is_err() method, which doesn't lead to that assumption.

But in that case, I guess this crate is redundant since rust-lang/rust#89926 ?

@taiki-e
Copy link
Owner

taiki-e commented May 16, 2022

dtolnay/request-for-implementation#27 and rust-lang/rust#58395 are the context of this crate.

I agree that none does not necessarily mean err, as it is up to the user to decide how to handle the results, but this crate was originally created with the use case of treating none as effectively err in mind.

(EDIT: To clarify, I'm not negative at all about the other use cases. I'm just concerned about a potential pitfall of the API for known use cases.)

But in that case, I guess this crate is redundant since rust-lang/rust#89926 ?

Well, that PR fixes the most annoying problem, but they are not the only operations that cause panic.

@dhardy
Copy link
Contributor Author

dhardy commented May 20, 2022

Updated. I probably won't end up using this library myself, so leave it to you to decide what to do with this.

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 30, 2022

Build succeeded:

@bors bors bot merged commit cc1ff54 into taiki-e:main Jun 30, 2022
@taiki-e
Copy link
Owner

taiki-e commented Jun 30, 2022

Published in 0.2.3.

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.

2 participants