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 conversion methods to toFoo #705

Merged
merged 27 commits into from
Jul 2, 2020
Merged

Rename conversion methods to toFoo #705

merged 27 commits into from
Jul 2, 2020

Conversation

ryzokuken
Copy link
Member

Fixes: #574.

/cc @ptomato @justingrant

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #705 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #705   +/-   ##
=======================================
  Coverage   92.33%   92.33%           
=======================================
  Files          17       17           
  Lines        4733     4733           
  Branches      743      743           
=======================================
  Hits         4370     4370           
  Misses        360      360           
  Partials        3        3           
Flag Coverage Δ
#test262 51.02% <20.00%> (ø)
#tests 87.11% <100.00%> (ø)
Impacted Files Coverage Δ
polyfill/lib/absolute.mjs 96.41% <100.00%> (ø)
polyfill/lib/date.mjs 90.90% <100.00%> (ø)
polyfill/lib/datetime.mjs 92.16% <100.00%> (ø)
polyfill/lib/intl.mjs 98.73% <100.00%> (ø)
polyfill/lib/monthday.mjs 93.42% <100.00%> (ø)
polyfill/lib/now.mjs 100.00% <100.00%> (ø)
polyfill/lib/time.mjs 96.61% <100.00%> (ø)
polyfill/lib/yearmonth.mjs 94.60% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9564d4c...e0a9d35. Read the comment docs.

@ptomato
Copy link
Collaborator

ptomato commented Jun 25, 2020

This looks fine, is it marked draft because you still intend to rename other methods as well?

@ryzokuken
Copy link
Member Author

@ptomato precisely.

@ryzokuken
Copy link
Member Author

Made all expected changes, rebased and now this is ready for review.

I'd like to note that it introduces a new ambiguity (now three methods, not two) between Absolute.prototype.toDateTime, Date.prototype.toDateTime and Time.prototype.toDateTime. However, I realized that it's much easier to keep track of things if the method is named after the return type, especially for long chained calls (which is something the current API tries to cater to). This was very evident after editing the cookbook examples. Things like Temporal.now.absolute().toDateTime(...).getDate().toDateTime(...).toAbsolute(...) just make so much more sense.

@justingrant
Copy link
Collaborator

Changes look good, but I was under the impression that we were also going to rename all getXxx methods to toXxx, e.g. DateTime.prototype.getDate->toDate. Are those changes coming later, or did we decide not to rename those "downgrade" methods?

(copying from #574 (comment))

Meeting, June 18: We'll rename everything to toTheTypeThatItConvertsTo. (inTimeZonetoAbsolute or toDateTime; withYeartoDate; getMonthDaytoMonthDay), and do this fairly quickly while still in the early stages of gathering feedback.

@ryzokuken
Copy link
Member Author

@justingrant oops, I totally forgot about those. Will do them today.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I didn't pay too much attention to the test262 part, but looks good, pending the other renames that you mentioned.

I would like to consider alternatives for YearMonth.toDate() and MonthDay.toDate() because I seriously think toDate(2020) and toDate(1) look confusing.

@@ -1,6 +1,6 @@
const birthday = Temporal.MonthDay.from('12-15');

const birthdayIn2030 = birthday.withYear(2030);
const birthdayIn2030 = birthday.toDate(2030);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, here is an example of where I really prefer the old names ... toDate(year) as below is OK, but toDate(2030) with a number literal is more mystifying to me. I know I was in the minority on that viewpoint so I definitely don't want to push for bringing the old names back; but maybe there is a third way? How about calling it toDateIn()? e.g. toDateIn(2030), toDateIn({ era: 'reiwa', year: 12 })

@@ -6,7 +6,7 @@
*/
function getFirstTuesday(queriedMonth) {
// We first need to convert to a date
const firstOfMonth = queriedMonth.withDay(1);
const firstOfMonth = queriedMonth.toDate(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, maybe toDateOn(1)? (although that reads to me as less obvious than toDateIn(2030))

docs/cookbook/meetingPlanner.js Outdated Show resolved Hide resolved
.withTime(Temporal.Time.from('00:00')) // midnight
.inTimeZone(here);
.toDateTime(Temporal.Time.from('00:00')) // midnight
.toDateTime(here);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be toAbsolute()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Will fix.

@ryzokuken
Copy link
Member Author

@ptomato that rebase was a little sad, with 29 commits and all, but it's finally done! Please take another look?

@ptomato
Copy link
Collaborator

ptomato commented Jul 1, 2020

This looks good, but I would really like to consider MonthDay.toDateIn(2020) and some similar rename for YearMonth.toDate().

@justingrant
Copy link
Collaborator

justingrant commented Jul 1, 2020

One pattern we could consider would be toXxxWith for conversions that require another argument, e.g. toDateWith({year: 2020}) or toDateTimeWith({timeZone: 'Europe/Paris'}). I'm not sure I like the verbosity, though.

@ryzokuken
Copy link
Member Author

ryzokuken commented Jul 2, 2020

@ptomato I have no strong preferences here, I think we're much better now in that:

  1. X.p.withFoo/X.p.with => X + Foo -> X.
  2. X.p.toBar => X (+ Y) -> Bar.

Feel free to make any suggestion for the methods that seem confusing to you.

@justingrant's suggestion of separating toBar like:

  1. X.toBar => X -> Bar
  2. X.toBarWith => X + Y -> Bar

makes sense. However, as he points it, it tends to get too verbose. Besides, I'm atleast slightly uncomfortable reusing with here.

@justingrant
Copy link
Collaborator

@ptomato - Given the scope of this PR and the challenge of rebasing it each time, would it make sense to merge as-is and then do a smaller follow-up PR to address naming for the two methods that you're concerned about?

@ryzokuken
Copy link
Member Author

@justingrant @ptomato squashed and rebased 😇

@justingrant
Copy link
Collaborator

@ptomato - your concerns about the YearMonth/MonthDay methods prompted me to propose #720 which doesn't fix the problems you found but might provide an alternative way to accomplish the same thing with clearer code.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

OK.

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.

Should Absolute and DateTime have different names for inTimeZone ?
3 participants