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 missing Float#weeks method similar to Int#weeks #7165

Merged
merged 1 commit into from
Dec 15, 2018
Merged

Add missing Float#weeks method similar to Int#weeks #7165

merged 1 commit into from
Dec 15, 2018

Conversation

vlazar
Copy link
Contributor

@vlazar vlazar commented Dec 9, 2018

Both Int and Float have days, hours,minutes, seconds, milliseconds, microseconds, nanoseconds.

On top of that Int also has weeks method, but Float does not.

@j8r
Copy link
Contributor

j8r commented Dec 9, 2018

Not directly related to this PR - it starts to have too many singular/plural aliases related to Timpe::Span 😟
I'll vote to keep the plural form, which is consistent with Int#times, and the method in Float which are all plural (thus for 1.0.days)

@wooster0
Copy link
Contributor

wooster0 commented Dec 9, 2018

Isn't Float#months and Float#years missing as well?

@vlazar
Copy link
Contributor Author

vlazar commented Dec 9, 2018

@r00ster91 You are right, but these return Time::MonthSpan, not Time::Span like all other methods (weeks .. nanoseconds) and Time::MonthSpan stores values as Int64, so unless that's changed the months and years for Float wouldn't make much sense.

@vlazar
Copy link
Contributor Author

vlazar commented Dec 9, 2018

Maybe there is a reason behind Time::MonthSpan not using floats. And if not, changing Time::MonthSpan implementation feels like a bigger change than just adding missing method(s). I guess it's better to discuss it first and address in separate PR. But I'm happy to address it here, if there is a need for it of course.

@straight-shoota
Copy link
Member

MontSpan can't express a fraction of a month, that's why float months and years don't make much sense.
This requires an appropriate datatype, see #6522.

@RX14 RX14 merged commit f467810 into crystal-lang:master Dec 15, 2018
@RX14 RX14 added this to the 0.27.1 milestone Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants