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 compat for BigInt #3224

Merged
merged 8 commits into from Dec 24, 2018

Conversation

Projects
None yet
4 participants
@VFDan
Copy link
Contributor

VFDan commented Dec 17, 2018

The BigInt compatibility table is needed:

@jpmedley
Copy link
Contributor

jpmedley left a comment

Thank you for your help on this.

Show resolved Hide resolved BigInt.json Outdated
Show resolved Hide resolved BigInt.json Outdated
Show resolved Hide resolved BigInt.json Outdated
Show resolved Hide resolved BigInt.json Outdated
Show resolved Hide resolved BigInt.json Outdated

@ddbeck ddbeck added the data:js 📟 label Dec 18, 2018

Update BigInt.json
Fixed some errors: #3224.
@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 18, 2018

I added a commit to fix the errors.

Show resolved Hide resolved BigInt.json Outdated
@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 19, 2018

I added a commit with the Node.js compatibility, and put it in the correct folder

@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 19, 2018

I un-localized the URL.

@ddbeck
Copy link
Collaborator

ddbeck left a comment

A couple style fixes and this will be ready to merge. Thank you for your help on this, @VFDan!

Show resolved Hide resolved javascript/builtins/BigInt.json Outdated
Show resolved Hide resolved javascript/builtins/BigInt.json Outdated
Show resolved Hide resolved javascript/builtins/BigInt.json Outdated
Update BigInt.json
Fixed some style errors
@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 20, 2018

I added a commit with those style changes.

@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 20, 2018

Travis CI says: "

  File : javascript/builtins/BigInt.json
  Style – Error on line #1
    Actual:   {
    Expected: {

"
But when I look at the file, there is nothing to the left of the {. I tried to backspace, but it did nothing.

@ddbeck

This comment has been minimized.

Copy link
Collaborator

ddbeck commented Dec 21, 2018

It appears that your line endings aren't just new lines (i.e., your line endings are Windows rather than Unix style). I've opened PR #3231 to highlight this better. In the mean time, there's probably an option in your editor to convert line endings, which should fix the problem.

@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 22, 2018

I changed the line endings (I said Unicode but meant Unix). Travis CI said the same thing. Maybe there is a bug with Travis CI, because I changed the line endings

ddbeck added some commits Dec 24, 2018

@ddbeck ddbeck dismissed their stale review Dec 24, 2018

Fixed!

@ddbeck

This comment has been minimized.

Copy link
Collaborator

ddbeck commented Dec 24, 2018

@VFDan I went ahead and fixed the line endings directly (and unnoticed tabs vs spaces issue). I don't know why that had to be such a hassle, but hopefully #3231 will avoid that in the future. I'll be merging your contribution shortly. Thank you! 🎉

@ddbeck ddbeck merged commit c7e3a3b into mdn:master Dec 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Dec 25, 2018

Thank you for merging the commits! I'm also on MDN under the username VFDan.

@mdn mdn deleted a comment from QuyenNguyenCDA Jan 16, 2019

@VFDan

This comment has been minimized.

Copy link
Contributor Author

VFDan commented Jan 28, 2019

I put the compatibility table in the MDN BigInt page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment