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

Add Math Extensions proposal to Stage 1 TC39 0716 #227

Merged
merged 1 commit into from
Aug 28, 2016
Merged

Add Math Extensions proposal to Stage 1 TC39 0716 #227

merged 1 commit into from
Aug 28, 2016

Conversation

clavecoder
Copy link
Contributor

@clavecoder clavecoder commented Jul 31, 2016

Implementation is based on the polyfill submitted along with the proposal.

[edit] The related TC39 meeting notes have dropped:
https://github.com/rwaldron/tc39-notes/blob/master/es7/2016-07/jul-26.md#9iie-math-extensions

This is my first PR, so it will need some work. I modeled the changes off of Number.epsilon and Math.umulh. The tests pass. Below are a list of open items. Please inform any other ways I can improve my submissions.

Open items:

  1. The README doesn't specify the version, of course.
  2. The spec also creates two well-known intrinsic objects %Math_max%, %Math_min%. Can they, should they be modeled in some way? They are used in the specification of Math.clamp.
  3. For the following files the computation of the constants was exported rather than the constant itself, following the pattern of Math.umulh. Should the identifier be exported? If so, how?
    • library/fn/math/deg-per-rad.js
    • library/fn/math/rad-per-deg.js
  4. In the following, I'm invoking Math.max, Math.min, and Math.PI directly. Should I create variables instead, as seems to be the pattern?
    • library/modules/es7.math.clamp.js
    • library/modules/es7.math.deg-per-rad.js
    • library/modules/es7.math.rad-per-deg.js
    • library/modules/es7.math.radians.js
    • modules/es7.math.clamp.js
    • modules/es7.math.deg-per-rad.js
    • modules/es7.math.rad-per-deg.js
  5. In the following, I haven't been able to import constants and functions defined by core.js, what is the way to do so?
    • library/modules/es7.math.degrees.js
    • library/modules/es7.math.fscale.js (including a call to scale)
    • library/modules/es7.math.scale.js
    • library/modules/es7.math.radians.js
    • modules/es7.math.degrees.js
    • modules/es7.math.fscale.js (including a call to scale)
    • modules/es7.math.scale.js
    • modules/es7.math.radians.js
  6. Looks like I need to add some <CR>s at the end of some files.

@zloirock
Copy link
Owner

zloirock commented Jul 31, 2016

Thanks. I'll completely review it little later, at this moment some comments.

@@ -176,12 +176,11 @@
__webpack_require__(225);
Copy link
Owner

Choose a reason for hiding this comment

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

Can you exclude browser bundles from this PR? It will be updated only at release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's everything under client/, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (previously)

@clavecoder
Copy link
Contributor Author

Changes per requested. Couple new git commands learned too, 😃.

@@ -0,0 +1,4 @@
// https://www.npmjs.com/package/ecma-proposal-math-extensions
Copy link
Owner

@zloirock zloirock Aug 2, 2016

Choose a reason for hiding this comment

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

Use https://rwaldron.github.io/proposal-math-extensions/ link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@clavecoder
Copy link
Contributor Author

clavecoder commented Aug 3, 2016

@zloirock, I've updated for your second round of comments. It should be in order now. I used Math.PI for the fscale/fround test.

@zloirock
Copy link
Owner

zloirock commented Aug 4, 2016

cc @rwaldron what do you think about it? Mainly interesting your position about Math.fscale. It's missed in the spec, but in opened questions and in your implementation.

@clavecoder
Copy link
Contributor Author

@zloirock , @rwaldron, Any updates on this Math-Extensions pr?

@zloirock
Copy link
Owner

@clavecoder I wanted at first to have a feedback from proposal's champion, but if it's missing... I'll merge it and publish a new release when I'll have some free time.

@clavecoder
Copy link
Contributor Author

Great, I'll try to move on to adding a PR for the more important #225 on Saturday.

@zloirock zloirock merged commit e6f3290 into zloirock:master Aug 28, 2016
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.

2 participants