-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fixed #635 and added unit tests #686
Conversation
CLA Assistant Lite bot Thank you for your contribution! Like many free software projects, you must sign our Contributor License Agreement before we can accept your contribution. EDIT: All contributors have signed quick-lint-js' Contributor License Agreement (CLA-v1.md). |
I have read and hereby agree to quick-lint-js' Contributor License Agreement (CLA-v1.md). |
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.
Looks good to me, but the code could be shorter.
Let me know when I should merge.
@@ -375,6 +375,7 @@ bool parser::parse_and_visit_statement( | |||
case token_type::slash_equal: // Regular expression. | |||
case token_type::string: | |||
case token_type::tilde: | |||
case token_type::less: |
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.
Nit: Sort alphabetically; move above line 371.
test/test-parse-statement.cpp
Outdated
u8"<div>hi</div>", | ||
u8"<a href=\"github.com/quick-lint/quick-lint-js\">link</a>", | ||
u8"<p></p>", | ||
u8"<div>this is <b>bold</b> and <i>italic</i> text</div>", | ||
u8"<head><title>head and title</title></head>", |
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.
Nit: I think we need only one or two examples of JSX, not five.
test/test-parse-statement.cpp
Outdated
}) { | ||
{ | ||
padded_string code(u8"return\n"s + second_line); | ||
SCOPED_TRACE(code); | ||
spy_visitor v; | ||
parser p(&code, &v); | ||
p.parse_and_visit_module(v); | ||
if (second_line[0] == '<') { |
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.
Let's always enable JSX for these tests. Make the test logic simple, not complex.
The CI failures aren't your fault. If you want to fix the CI errors, rebase or merge. |
Correctly reports E0179 when returning an html tag on a new line. Added unit tests as well.
Co-authored-by: strager <strager.nds@gmail.com>
Fixes the nitpicks strager had and is ready to merge.
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.
Looks great!
Previously, the code:
wouldn't report an error. It now correctly reports E0179 when returning an html tag on a new line. I also added unit tests.