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 end position to ParserError #157

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add end position to ParserError #157

wants to merge 1 commit into from

Conversation

tscpp
Copy link

@tscpp tscpp commented Nov 17, 2020

No description provided.

@tscpp tscpp marked this pull request as draft November 17, 2020 15:29
@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request introduces 7 alerts when merging 761366d into b907fc9 - view on LGTM.com

new alerts:

  • 6 for Superfluous trailing arguments
  • 1 for Unused variable, import, function or class

/**
* The source code to be parsed
*/
source: string
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the semicolon in interface def?

Copy link
Author

Choose a reason for hiding this comment

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

Not an actual reason. I'm just used to don't use semis.

Copy link
Member

Choose a reason for hiding this comment

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

But they are there for our other interface definitions. I suggest to put it back here for consistency.

I am surprised "prettier" does not have an opinion on this syntax, it accepted both. But there is semi: true in our prettier.config.js, a "prettier" bug?
If you want to cleanup the type def to remove semicolon, pls create another PR to touch all of the interfaces, if @KFlash has no strong opinion on this syntax thing.

Copy link
Member

Choose a reason for hiding this comment

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

@fisker should not semi:true in prettier.config.js enforce the semicolon here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, why don't we run prettier --check on CI?

Copy link
Member

@3cp 3cp Nov 22, 2020

Choose a reason for hiding this comment

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

Got it, we got these in .prettierignore. @KFlash why we ignored those source files?

bench/v1.6.2/
bench/v2/
bench/v8-native-calls.js
node_modules/
dist/
src/token.ts
src/common.ts
src/chars.ts
src/lexer/charClassifier.ts
src/unicode.ts
package-lock.json

Copy link
Contributor

Choose a reason for hiding this comment

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

the style get fucked up with Prettier and not lined. Regarding unicode.ts it will be a long list (2000 lines)

Copy link
Contributor

Choose a reason for hiding this comment

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

@3cp Possible to use non-open source to replace Prettier? I got my own pretty printer we could use, but ain't going to open source that one.

Copy link
Member

Choose a reason for hiding this comment

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

It could be weird for open source project to depend on non-open project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point!

export function report(parser: ParserState, type: Errors, ...params: string[]): never {
throw new ParseError(parser.index, parser.line, parser.column, type, ...params);
export function report(parser: ParserState, context: Context, node: Node, type: Errors, ...params: string[]): never {
const end = finishNode(parser, context, parser.index, parser.line, parser.column, node).loc?.end;
Copy link
Member

Choose a reason for hiding this comment

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

The finishNode mutates nothing on parser: ParserState. If you look into the implementation, the loc.end is just

end: {
        line: parser.startLine,
        column: parser.startColumn
      }

Which you can directly get from parser.
Also, the availability of loc.end in finishNode depends on OptionsLoc, but you can always get parser.startLine directly.

src/estree.ts Show resolved Hide resolved
src/parser.ts Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
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.

None yet

4 participants