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

Merged
merged 8 commits into from
Dec 24, 2018
Merged

Add compat for BigInt #3224

merged 8 commits into from
Dec 24, 2018

Conversation

VFDan
Copy link
Contributor

@VFDan VFDan commented Dec 17, 2018

The BigInt compatibility table is needed:

Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

Thank you for your help on this.

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 Show resolved Hide resolved
@ddbeck ddbeck added the data:js 📟 Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript label Dec 18, 2018
Fixed some errors: #3224.
@VFDan
Copy link
Contributor Author

VFDan commented Dec 18, 2018

I added a commit to fix the errors.

BigInt.json Outdated Show resolved Hide resolved
@VFDan
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
Copy link
Contributor Author

VFDan commented Dec 19, 2018

I un-localized the URL.

ddbeck
ddbeck previously requested changes Dec 20, 2018
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

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

javascript/builtins/BigInt.json Outdated Show resolved Hide resolved
javascript/builtins/BigInt.json Outdated Show resolved Hide resolved
javascript/builtins/BigInt.json Outdated Show resolved Hide resolved
Fixed some style errors
@VFDan
Copy link
Contributor Author

VFDan commented Dec 20, 2018

I added a commit with those style changes.

@VFDan
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
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
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
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
@VFDan
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 Jan 16, 2019
@VFDan
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
Labels
data:js 📟 Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants