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

Delegate review: Bradley Farias #1383

Closed
ptomato opened this issue Feb 25, 2021 · 3 comments
Closed

Delegate review: Bradley Farias #1383

ptomato opened this issue Feb 25, 2021 · 3 comments
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Feb 25, 2021

Thread for discussions / signing off from designated reviewer @bmeck, in preparation for Stage 3

@ptomato ptomato added this to the Stage 3 milestone Feb 25, 2021
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 25, 2021

@bmeck thanks for joining the Temporal meeting today!

Here are the actionable items that I distilled from your comments during the meeting:

  • In the parts that mention the "time zone information" being updated, it's ambiguous whether this means the information about which time zone the system is in, or the information in the time zone database. It's the latter and should be changed accordingly.
  • Remove references to "immutable Object" from the text as it doesn't have a definition within the specification, Temporal objects are only "immutable" from the user's perspective.
  • In cases where an options object (from NormalizeOptionsObject) is passed to a non-ISO-calendar operation (which is implementation-defined and not part of the specification) then it still needs to be specified in which order the properties of the options object are accessed.
  • Possible name change for abstract operations to make clear that they concern only the ISO calendar (e.g., RejectDate(year, month, day) → RejectISODate(year, month, day) or RejectDate(isoYear, isoMonth, isoDay))
  • I'll notify you on the upcoming change from delegate review: Calendar and TimeZone should be brand-checked, not normal, objects #1294 about how calendar objects are stored in internal slots.
  • Examine operations where property reads are interspersed with operations that may throw.
  • Ensure Get vs GetOwnProperty are used with the corresponding enumeration, and don't use own keys.

If you don't mind, I'll go ahead and open issues for the first four. The last two, are they things that you intend to look out for while completing your review or are there things that you already feel should be changed?

@ptomato
Copy link
Collaborator Author

ptomato commented Dec 8, 2022

Nothing left to do here.

@ptomato ptomato closed this as completed Dec 8, 2022
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

1 participant