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

remove observable calls to "from" #1419

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

ryzokuken
Copy link
Member

Fixes: #1293

/cc @ptomato @ljharb

@ryzokuken ryzokuken requested a review from ptomato March 5, 2021 23:32
@ryzokuken ryzokuken self-assigned this Mar 5, 2021
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #1419 (ba60eb9) into main (dccb08f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   95.73%   95.75%   +0.02%     
==========================================
  Files          19       19              
  Lines       11143    11129      -14     
  Branches     1804     1801       -3     
==========================================
- Hits        10668    10657      -11     
+ Misses        472      469       -3     
  Partials        3        3              
Flag Coverage Δ
test262 48.93% <37.50%> (-0.09%) ⬇️
tests 91.65% <90.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
polyfill/lib/calendar.mjs 94.07% <100.00%> (-0.10%) ⬇️
polyfill/lib/ecmascript.mjs 96.24% <100.00%> (+0.10%) ⬆️
polyfill/lib/timezone.mjs 93.37% <100.00%> (-0.38%) ⬇️

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 dccb08f...955c9d2. Read the comment docs.

spec/calendar.html Show resolved Hide resolved
1. If Type(_item_) is Object, then
1. If ? HasProperty(_item_, *"timeZone"*) is *false*, return _item_.
1. Set _item_ to ? Get(_item_, *"timeZone"*).
1. If Type(_item_) is Object and ? HasProperty(_item_, *"timeZone"*) is *false*, return _item_.
Copy link
Member

Choose a reason for hiding this comment

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

same here

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.

Thanks! A few changes still to be made. I think it's fine to un-"draft" this PR by the way.

polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
polyfill/test/regex.mjs Show resolved Hide resolved
spec/calendar.html Show resolved Hide resolved
This commit removes observable "from" lookups for `Calendar` and
`TimeZone` from the spec and the polyfill, in response to the delegate
reviews.

Refs: tc39#1293
`from` methods can no longer be monkey-patched, so this commit removes
that behavior from the cookbook documentation.
The polyfill no longer does observable `from` lookups for `Calendar` and
`TimeZone`. Therefore, this commit removes tests that check that
behavior and modifies those that merely rely on it.
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.

Thanks, CreateTemporalCalendarFromStatic is the way to go.

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.

delegate review: observable "from" lookups
3 participants