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

Jump to bottom

Rephrase parser error message #11208

Merged
merged 6 commits into from Mar 17, 2020
Merged

Rephrase parser error message #11208

merged 6 commits into from Mar 17, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Mar 4, 2020

Q                       A
Tests Added + Pass? Yes
License MIT

Rephrase some parser error messages and resolve todos introduced in #11192 . PTAL and comment if you have any ideas. 🙏

(Yes we will do todos)

@JLHwung JLHwung added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: parser labels Mar 4, 2020
@JLHwung
Copy link
Contributor Author

JLHwung commented Mar 4, 2020

The codecov error is a false negative. This time codecov returns different HTTP code when a repository token secret is not available.

This one returns 400 (expected) https://github.com/babel/babel/pull/11201/checks?check_run_id=483631598#step:6:37

This one returns some code other than 400
https://github.com/babel/babel/runs/486123611?check_suite_focus=true#step:6:37

I will add an empty commit tomorrow to see if it is fixed.

nicolo-ribaudo
nicolo-ribaudo reviewed Mar 4, 2020
View reviewed changes
packages/babel-parser/src/parser/location.js
UnsupportedParameterDecorator:
"Stage 2 decorators cannot be used to decorate parameters",
"Decorators cannot be used to decorate parameters",
Copy link
Member

Choose a reason for hiding this comment

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

Actually, legacy decorators can be used to decorate parameters. (for TS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is okay because this error is not visible to users if they are using legacy decorators. Like we did in "Legacy octal literals are not allowed in strict mode", if an error is thrown on a legacy elements, we should prefix its name. Otherwise we are referring to the latest proposal.

nicolo-ribaudo
nicolo-ribaudo reviewed Mar 4, 2020
View reviewed changes
packages/babel-parser/src/tokenizer/index.js Outdated
// todo: merge with Errors.StrictOctalLiteral
"Legacy octal literals are not allowed in strict mode",
);
this.raise(start, Errors.StrictOctalLiteral);
Copy link
Member

Choose a reason for hiding this comment

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

Here we might want to change the StrictOctalLiteral message, because 0o1 is perfectly valid in strict mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, 0o1 is octal literal but not legacy octal literals. I have changed StrictOctalLiteral to be "Legacy octal literals are not allowed in strict mode".

@nicolo-ribaudo
Copy link
Member

@kaicataldo FYI, this probably conflicts with your PR.

@JLHwung JLHwung force-pushed the rephrase-parser-error-message branch from 2a59a6e to 74bc15f Compare March 11, 2020 22:04
@nicolo-ribaudo nicolo-ribaudo added PR: Polish 💅 A type of pull request used for our changelog categories and removed PR: Internal 🏠 A type of pull request used for our changelog categories labels Mar 11, 2020
@JLHwung JLHwung force-pushed the rephrase-parser-error-message branch from 74bc15f to 4a79c1e Compare March 12, 2020 21:08
@JLHwung JLHwung force-pushed the rephrase-parser-error-message branch from ae77e40 to 9d032b5 Compare March 17, 2020 01:47
@JLHwung JLHwung merged commit 841f442 into babel:master Mar 17, 2020
This was referenced Mar 22, 2020
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Mar 30, 2020
Open
Open
This was referenced Apr 12, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 20, 2020
Open
Open
Open
Open
Open
Open
This was referenced Apr 27, 2020
Closed
Open
Open
Merged
Open
Merged
Open
Closed
This was referenced May 5, 2020
Open
Open
Open
Open
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@existentialism existentialism existentialism approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JLHwung @nicolo-ribaudo @existentialism