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

Rename MonthDay.toDate() and YearMonth.toDate() #722

Closed
ptomato opened this issue Jul 2, 2020 · 6 comments · Fixed by #723
Closed

Rename MonthDay.toDate() and YearMonth.toDate() #722

ptomato opened this issue Jul 2, 2020 · 6 comments · Fixed by #723

Comments

@ptomato
Copy link
Collaborator

ptomato commented Jul 2, 2020

I find these two particular toDate() methods to read confusingly when used with a number literal:

monthDay.toDate(2020)
yearMonth.toDate(1)

I think these two are special cases, different than the other conversion methods, because they are the only ones that can be used with number literals.

I would propose renaming MonthDay.toDate() to MonthDay.toDateIn(), which reads like a sentence ("date in 2020"). I don't have a good suggestion for YearMonth.toDate(), but the best I can think of is YearMonth.toDateOn(), analogous to toDateIn(). Unlike toDateIn() it doesn't read like a sentence, but it at least gives a clue:

monthDay.toDateIn(2020)
monthDay.toDateIn({ era: 'reiwa', year: 2 })  // also reads like a sentence
yearMonth.toDateOn(1)
@ptomato ptomato added this to the Stable API milestone Jul 2, 2020
ptomato added a commit that referenced this issue Jul 2, 2020
This is in order to reduce confusion when using the method with a number
literal.

See: #722
ptomato added a commit that referenced this issue Jul 2, 2020
This is in order to reduce confusion when using the method with a number
literal.

See: #722
@ptomato ptomato self-assigned this Jul 2, 2020
ptomato added a commit that referenced this issue Jul 6, 2020
This is in order to reduce confusion when using the method with a number
literal.

See: #722
ptomato added a commit that referenced this issue Jul 6, 2020
This is in order to reduce confusion when using the method with a number
literal.

See: #722
@thojanssens
Copy link

thojanssens commented Jul 7, 2020

Hi. New to this project.
Then this issue makes me wonder why we don't follow the java's API:

  • date.atTime
  • time.atDate

Then for YearMonth we can use:

  • yearMonth.onDay

and for MonthDay:

  • monthDay.inYear

(Actually Java uses atDay, atYear. I'm not native, but if that works that's even better to keep consistency.)

I guess this has already been discussed a lot:-) but before this issue came up.

@thojanssens
Copy link

Or, as we have a with function already:

withTime
withDate
withDay
withYear

but for issue #729, there would be
withDayAtEndOfMonth
withTimeAtStartOfDay
which isn't so bad.

@ptomato
Copy link
Collaborator Author

ptomato commented Jul 7, 2020

Hi. New to this project.

Then welcome, and thanks for your feedback here and on the other tickets you've opened!

Until recently, those methods were in fact named withTime, etc. The full background of the discussion is in #574, but the summary is that the experience in an IDE is much better when you can type to and see a list of methods that are named after the type they convert to. We believe this would help people figure out faster which type they need to use.

That said, the naming is of course still open to discussion, so I will reopen that issue with the "feedback" tag so that we will re-examine it before freezing the API. What would be for you the motivation for changing back to withTime/withDate/etc.?

@justingrant
Copy link
Collaborator

Welcome @thojanssens! One addition to @ptomato's note above: in addition to IDE support, another reason for the toXxx naming convention was the assumption that many users would have a similar experience that I did when first starting to use Temporal. From #574 (comment):

After a few days of using Temporal whenever I needed to do a type conversion I always thought first about the type I wanted and only secondarily about the parameter types I needed to use to get it. I found it more natural to focus on ends over means. It's also why I find "inTimeZone" so frustrating-- because I'm not trying to get a TimeZone, I'm trying to get a DateTime or Absolute!

@thojanssens
Copy link

thojanssens commented Jul 8, 2020

It deserves another thought as the talks happened before the possible addition of:
toDateOnDay
toDateInYear
And maybe
toDateAtEndOfMonth
toDateTimeAtStartOfDay (and should I use at or on as above.. we have to remember the prefix used)

If using the at- prefix, it becomes more simply atDay, atYear, atEndOfMonth, atStartOfDay, in addition to atDate and atTime to convert to Time or Date to a DateTime.
Using with- will result in withDay, withYear, withDayAtEndOfMonth, withTimeAtStartOfDay, in addition to withDate and withTime to convert to Time or Date to a DateTime.

I understand though the atTimeZone for an Absolute or DateTime is confusing and wrong. But for me, it seems a special case, not like atDate, atTime, and the ones above. Let me explain:

For example, I can naturally understand that a Date combined with a Time gives a DateTime.
A YearMonth with a Day gives a Date. Etc.
You know that without looking at the docs.

BUT an Absolute and a Time zone? What does that give? Well naturally I would say it gives exactly that, an Absolute combined with a Time zone. Not a DateTime without time zone. Same question, what is a DateTime with a Time zone? It is a "zoned date time", right? But I have to look at the docs to understand that I actually get an Absolute, and again losing the time zone information.

Therefore, to me, inTimeZone is wrong in any case; totally agree with that. toDateTime and toAbsolute, would be the way to go for the current proposed objects.
But again, these cases are different than the ones above. So I'm not sure that they should influence the naming for the first functions mentioned.
at- and with- give more flexibility in adding methods. toDateTimeAtStartOfDay is long.

toDateOnDay and toDateInYear result in two different prefixes to remember: once it's on, and once it's in, ...

I'd like you to consider having at- or with- functions, but keeping toDateTime and toAbsolute for converting Absolute <-> DateTime as those last two functions don't "remember" the time zone provided, as opposed to how the other functions work.

@ptomato
Copy link
Collaborator Author

ptomato commented Jul 10, 2020

@thojanssens Since this issue will auto-close when I merge #723, I've opened #747 to continue the discussion, you may want to subscribe there. Justin opened #737 for the discussion about atStartOfDay and friends. (oops, that one was for midnight specifically; the one I actually wanted was #729 which you opened yourself already.)

ptomato added a commit that referenced this issue Jul 10, 2020
This is in order to reduce confusion when using the method with a number
literal.

See: #722
ptomato added a commit that referenced this issue Jul 10, 2020
This is in order to reduce confusion when using the method with a number
literal.

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

Successfully merging a pull request may close this issue.

3 participants