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

Document that with methods accept Temporal object instances #713

Closed
justingrant opened this issue Jun 27, 2020 · 1 comment · Fixed by #714
Closed

Document that with methods accept Temporal object instances #713

justingrant opened this issue Jun 27, 2020 · 1 comment · Fixed by #714
Assignees
Labels
documentation Additions to documentation

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jun 27, 2020

Because in #403 it was decided that fields shouldn't be enumerable, I assumed that code like the following was required to use with with Temporal object instances.

const dt = Temporal.now.dateTime();
const midnight = Temporal.Time.from('00:00');
const newYear = Temporal.MonthDay.from({month: 1, day: 1});

const todayMidnight = dt.with({...midnight.getFields()});
const thisJan1 = dt.with({...newYear.getFields()});

This assumption was wrong, which seems obvious now. But it still took me a few weeks to realize that I can use this much easier and intuitive code:

// much easier!
const todayMidnight = dt.with(midnight);
const thisJan1 = dt.with(newYear);

// the same trick works with `from` too!  (this is already documented though)
Temporal.DateTime.from(Temporal.now.date())
Temporal.Time.from(Temporal.now.dateTime())

In a quick look at the docs for with, I only saw example code with object literals, which made this trick harder to discover. I think I'd slotted these methods into a mental box that said "only accepts object literals". This is obviously wrong in retrospect but is a good example of how unintended messages in the docs can persist in novice developers' heads.

IMHO, it's worth adding additional samples to documentation for with to illustrate that even though you can't spread a Temporal instance into an object, you can pass it into with of another compatible Temporal instance.

@ptomato
Copy link
Collaborator

ptomato commented Jun 30, 2020

I think the cases that are worth documenting are:

  • Date.with(YearMonth)
  • Date.with(MonthDay)
  • DateTime.with(Time)
  • DateTime.with(Date)
  • DateTime.with(YearMonth)
  • DateTime.with(MonthDay)

Anything else, we should probably not be encouraging, as it would overwrite all the fields and should probably better use from() or one of the type conversion methods.

ptomato added a commit that referenced this issue Jun 30, 2020
The cases where it could make sense to pass a Temporal object:

- Date.with(YearMonth)
- Date.with(MonthDay)
- DateTime.with(Time)
- DateTime.with(Date)
- DateTime.with(YearMonth)
- DateTime.with(MonthDay)

Closes: #713
ryzokuken pushed a commit that referenced this issue Jul 1, 2020
The cases where it could make sense to pass a Temporal object:

- Date.with(YearMonth)
- Date.with(MonthDay)
- DateTime.with(Time)
- DateTime.with(Date)
- DateTime.with(YearMonth)
- DateTime.with(MonthDay)

Closes: #713
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants