fix: new diag for async/generator on getter/setter #1087
Conversation
koopiehoop
force-pushed
the
new_diag_async_or_generator_on_getter_or_setter
branch
from
8462f95 to
343c3d5
Compare
koopiehoop
force-pushed
the
new_diag_async_or_generator_on_getter_or_setter
branch
from
0c145d8 to
a8f6515
Compare
There was a problem hiding this comment.
Did I place [the tests] right
Yes!
do [the tests] need
Spy_VisitorwithEXPECT_THAT(p.visits, ...)?
No. What you did is great.
And what do you think about code repetition
We could parameterize the function (parameter for Token_Type::star/Token_Type::kw_async + template parameter for Diag_Class_Generator_On_Getter_Or_Setter/Diag_Class_Async_On_Getter_Or_Setter), but it might not be worth the effort.
| [[qljs::message("'*' keyword is not allowed on getters or setters", | ||
| ARG(generator_keyword))]] // |
There was a problem hiding this comment.
I think this is a confusing message. "'*' keyword" doesn't make sense because * is not a keyword.
I think a better phrasing is: "getters and setters cannot be generators"
| [[qljs::message("'*' keyword is not allowed on getters or setters", | ||
| ARG(generator_keyword))]] // | ||
| [[qljs::message("'{0}' here", ARG(getter_setter_keyword))]] // | ||
| Source_Code_Span method_start; |
There was a problem hiding this comment.
Nit: I think method_start is unnecessary, so I'd remove it.
src/quick-lint-js/fe/parse-class.cpp
Outdated
| error_if_generator_method(); | ||
| error_if_async_method(); |
There was a problem hiding this comment.
Nit: I think "error if async method" is a misleading name. "error if async getter or setter" is more appropriate.
| error_if_generator_method(); | |
| error_if_async_method(); | |
| error_if_generator_getter_or_setter(); | |
| error_if_async_getter_or_setter(); |
| u8" ^^^ .getter_setter_keyword"_diag, | ||
| javascript_options); | ||
| test_parse_and_visit_statement( | ||
| u8"class C { async get myMethod() { } }"_sv, // |
|
This patch looks good to me. Let me know when I should merge it. You can ignore the CI failures. I disconnected my ARM build bot (in my home lab) because I'm moving. It'll get hooked up in a few weeks. |
koopiehoop
force-pushed
the
new_diag_async_or_generator_on_getter_or_setter
branch
from
fd0edfb to
570759c
Compare
All suggested fixes have been applied, if the docs for the site looks good, then you can merge |
|
Landed as commit be755cb. Thanks! |
closes #1066
Can you check new tests? Did I place them right and do they need
Spy_VisitorwithEXPECT_THAT(p.visits, ...)?And what do you think about code repetition (
error_if_generator_method,error_if_async_method)? Looks like this is the only clean solution without compromising readability