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.

Sign up for GitHub

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

Feat: add shebang handling to lexer and parser #43

Merged
merged 15 commits into from Oct 13, 2020
Merged

Feat: add shebang handling to lexer and parser #43

merged 15 commits into from Oct 13, 2020

Conversation

magurotuna
Copy link
Contributor

Closes #26
This PR adds handling of shebang for lexer and parser, and also updates the corresponding change logs.

I'm not sure of the following things:

  • When running cargo xtask syntax after fixing xtask/src/ast.rs, I saw Cargo.lock was updated as well. I committed the change to git. Should the change of Cargo.lock be committed?
  • I didn't use Lexer::strip_shebang for this patch, instead created the other method Lexer::read_shebang. Should Lexer::strip_shebang be removed?

RDambrosio016
RDambrosio016 reviewed Oct 12, 2020
View reviewed changes
Copy link
Collaborator

@RDambrosio016 RDambrosio016 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 the PR!
Looks good to me either than a few comments 👍

crates/rslint_lexer/src/lib.rs Outdated Show resolved Hide resolved
crates/rslint_lexer/src/lib.rs Outdated Show resolved Hide resolved
crates/rslint_lexer/src/lib.rs Outdated Show resolved Hide resolved
crates/rslint_lexer/src/lib.rs Outdated Show resolved Hide resolved
crates/rslint_lexer/src/tests.rs Outdated Show resolved Hide resolved
@RDambrosio016
Copy link
Collaborator

1: Yes any changes to the lockfile should be committed
2: yeah strip_shebang can just be removed since it was a hack and shebangs are technically part of the grammar

@RDambrosio016 RDambrosio016 added T-Lexer This issue primary relates to rslint_lexer T-Parser This issue primarily relates to rslint_parser labels Oct 12, 2020
@magurotuna
Copy link
Contributor Author

@RDambrosio016
Thanks for the excellent detailed review!
I think I've addressed what you pointed out, could you please review it again?

Now I'm a bit wondering if there should be a test for a case where # isn't located at the beginning.

RDambrosio016
RDambrosio016 approved these changes Oct 13, 2020
View reviewed changes
Copy link
Collaborator

@RDambrosio016 RDambrosio016 left a comment

Choose a reason for hiding this comment

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

LGTM, just a small changelog thing and could you add some tests for # and # not oncurring at the start?

crates/rslint_lexer/CHANGELOG.md Show resolved Hide resolved
@magurotuna
Copy link
Contributor Author

Thanks again. Added more tests, and updated the changelog.

@RDambrosio016
Copy link
Collaborator

LGTM, thank you

@RDambrosio016 RDambrosio016 merged commit 4d2eaf1 into rslint:master Oct 13, 2020
@magurotuna magurotuna deleted the shebang branch October 13, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RDambrosio016 RDambrosio016 RDambrosio016 approved these changes

Assignees
No one assigned
Labels
T-Lexer This issue primary relates to rslint_lexer T-Parser This issue primarily relates to rslint_parser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shebangs should be parsed correctly
2 participants
@magurotuna @RDambrosio016