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

Small Issues before 1.0 #1

Open
hummingly opened this issue May 24, 2019 · 2 comments
Open

Small Issues before 1.0 #1

hummingly opened this issue May 24, 2019 · 2 comments
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@hummingly
Copy link

hummingly commented May 24, 2019

Hey I was the person who reviewed your crate on Reddit :)

I looked at your changes and one thing made me curious. You're using now a NonZeroI8 instead of a u8 which would mean there could be also negative days. If that isn't what you intended, I would recommend using a NonZeroU8.
Another thing that could be a little bit annoying for users is the conversion from u64 to Day. Any conversion that could fail shouldn't use the From trait but rather the TryFrom one.

Hope that helps towards 1.0!

@heca-project
Copy link
Owner

Thanks a lot!

If that isn't what you intended, I would recommend using a NonZeroU8.

Yes. Thanks for pointing that out. I'll probably have to change that, and may have to bump to a 2.0.

Another thing that could be a little bit annoying for users is the conversion from u64 to Day. Any conversion that could fail shouldn't use the From trait but rather the TryFrom one.

Also true, this was written before TryFrom was stabilized.

@heca-project
Copy link
Owner

Although, since I have to make verify the amount of days per month anyways (sometimes it's 29 and sometimes it's 30), for the time being I can just say that a negative day is a ConversionError and put the API change on hold for now.

@heca-project heca-project added this to the 2.0 milestone May 26, 2019
@heca-project heca-project added enhancement New feature or request bug Something isn't working labels May 26, 2019
heca-project added a commit that referenced this issue May 26, 2019
heca-project added a commit that referenced this issue Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants