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

[RFC] Time and Span don't have the same interface for inspection #9002

Open
juanboca opened this issue Apr 3, 2020 · 12 comments
Open

[RFC] Time and Span don't have the same interface for inspection #9002

juanboca opened this issue Apr 3, 2020 · 12 comments

Comments

@juanboca
Copy link

juanboca commented Apr 3, 2020

While starting to use Time and Span for the first time, I encountered with some compiler errors while trying to inspect the result of certain operations. Trying to apply minute to a variable sometimes worked, sometimes gave an error.

This was due to the different class returned by each of this operations involving Time and Span

  • Time - Time gives a Time::Span
  • Time - Time::Span gives a Time
  • Time::Span - Time::Span gives a Time::Span

I understand that this may come from Mono, but is there a reason why we might wan to keep it like this for 1.0?

This happens with day, hour, minute and second

@straight-shoota
Copy link
Member

I don't understand what's the problem here.
What do you mean by "apply minute to a variable"? Can you show an example of code and want you expected it to do?

The subtraction results seem quite logical to me, this is just basic math. Time represents an instant on the time line and Time::Span an interval. The difference between two instants is an interval. Subtracting an interval from an instant results in another instant. And the difference between two intervals is another interval.

@juanboca
Copy link
Author

juanboca commented Apr 5, 2020

You are right. Reading it now I wasn't very clear about what I meant.

The issue I'm trying to address is that the inspection methods for Time are in singular (day, hour...) and for Time::Span are in plural (days, hours).

Of course I have no issue with the subtraction results and the returned class for each one.
What I was trying to highlight is that very early on my first approach to using dates in Crystal I needed to memorize those returned classes in order to properly inspect the result, and that felt strange.

@straight-shoota
Copy link
Member

The singular vs. plural is on purpose: In the usual representation of instances based on time units, each one of them represents a proportional component, such as the second, the minute, the hour, the day, the month, the year of that instance. That's why the methods are in singular form.
A time span spans a number of seconds, minutes, hours, days, months, years. Thus the methods are in plural from.

@bcardiff
Copy link
Member

bcardiff commented Apr 5, 2020

Maybe my non-native english is getting in the middle, but Time::Span#minutes returns the minute component of the whole time span represented in the normalized(?) form. I understand that total_minutes is plural. I too feel weird that is Time#minute and Time::Span#minutes.

I also consider that you should be dealing with one or the other type usually, but having two similar, yet different APIs for related types is a bit confusing IMO.

Time::Span.new(minutes: 60).hours # => 1
Time::Span.new(minutes: 60).minutes # => 0
Time::Span.new(minutes: 60).seconds # => 0
Time::Span.new(minutes: 60).total_minutes # => 60.0
Time::Span.new(minutes: 60).total_seconds # => 3600.0

@straight-shoota
Copy link
Member

I don't think this is any different in other languages. In spanish for example you ask "¿Qué hora es?" for the time and "¿Cuantas horas?" for a time span.
And IMO it should actually be considered a feature that the accessor methods in Time and Time::Span have different names, because they're different concepts. It makes it easier to tell both data types apart.

That Time::Span.new(minutes: 60).minutes doesn't return 60 is related to #6522

@bcardiff
Copy link
Member

bcardiff commented Apr 8, 2020

I don't follow why you say they are a different concept @straight-shoota. I understand that a Time::Span is not a Time, but to me both #minute and #minutes return the mm components when the values are expressed in a human friendly format: HH:mm:ss.

I also fail to see the relationship with #6522. Even if the amount and unit are preserved the #minutes method should still match my previous statement IMO.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 8, 2020

  • Time represents an instant and can be expressed as a human readable format including a minute component which describes the minute of the hour.
  • Time::Span represents an interval and can be expressed as a human readable format including a minutes component which describes the amount of minutes in this interval.

Okay, so you're just saying that Time::Span#minutes should always return the total amount of minutes in the interval, not just the minutes component. So it should be swapped with #total_minutes, and the other would be #minutes_component?

@asterite
Copy link
Member

asterite commented Apr 8, 2020

I think for a time span you can say: 2, hours, 3 minutes, 5 seconds. Then if you ask "how many minutes" it will say "3". But if you ask "how many total minutes there are" it's a different question.

That's at least what the API is modeling, and we copied it from C#. Whether it's good or bad, I don't know.

@waj
Copy link
Member

waj commented Apr 8, 2020

No, I don't think @bcardiff is saying that. I think you're just wordplaying to make it fit with the current definitions. You can also say that in Time there is a minute component that describes the number of minutes since the hour.

Also, Time represents the interval of time since the day 0 (let's keep religious topics aside). So there is not such thing as "absolute time". It's purely conventional.

@straight-shoota
Copy link
Member

I'm pretty sure time would still exist even if nobody had invented calendars and clocks to keep track of it 😄 Without going too deep into philosophics, I think that should count as "absolute".

However, you're right that it's also legitimiate to describe the time of day as the interval since the beginning of the day. And applied to the smaller units, the minute component as the number of minutes elapsed since the beginning of the hour.
But in my understanding this is no longer only about time instances, but time intervals come into play. We're talking about an interval of time since the beginning of the hour.

@waj
Copy link
Member

waj commented Apr 8, 2020

Yes, I agree we shouldn't reinvent the theory of relativity in this thread 😆

And conventions or not, I agree that Time is trying to represent an absolute place in time. So I think that's what makes the singular methods appropriate: that particular combination of year/day/month/hour/minute/second it's a non repeatable entity.

@RX14
Copy link
Contributor

RX14 commented Apr 9, 2020

Since these are not the total_* methods, I think unifying their naming would be fine. They have similar enough semantics. I'd choose the plural version from Time::Span to stick with.

I don't think it matters much though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants