-
Notifications
You must be signed in to change notification settings - Fork 64
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 lint for header format #205
Conversation
@michaelficarra addressed comments. |
|
||
// CreateForInIterator | ||
// Object.fromEntries | ||
// Array.prototype [ @@iterator ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should support Array.prototype [ %Symbol.iterator% ]
as well, since we're moving to that form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that we disallow it now and make it legal as part of the PR which changes the format, so we are never in a position where both are accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's in a different repo. There's a currently open PR. How do you expect that to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd change it here, bump it, release it, and include the change in the upstream PR as part of the process of merging that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love it but I'll leave it to @ljharb to fight this fight.
let { line, column } = indexWithinElementToTrueLocation( | ||
getLocation(dom, element), | ||
contents, | ||
name.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name.length | |
name.length - 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. It feels kind of weird to point to the e
in Example( )
, but sure, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also point to the second space in
Example ( )
cf https://github.com/tc39/ecma262/pull/1998/files#diff-3540caefa502006d8a33cb1385720803R37872