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

feat: error on multiple export defaults in module #1096

Closed

Conversation

arieldon
Copy link
Contributor

This closes issue #1039.

@github-actions
Copy link

github-actions bot commented Oct 24, 2023

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).

@arieldon
Copy link
Contributor Author

I have read and hereby agree to quick-lint-js' Contributor License Agreement (CLA-v1.md).

Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

This looks great! Let me know if you want to make changes or if I should merge it now.

Comment on lines 3313 to 3314
[[qljs::message("cannot export default more than once", ARG(second_export_default))]] //
[[qljs::message("export default previously appeared here", ARG(first_export_default))]] //
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Message here should match the title in docs/errors/E0715.md. (It doesn't have to be an exact match.)

For example, you could title the docs "cannot export default more than once" or you could change the first message to "cannot use multiple export default statements in one module".

Comment on lines 1015 to 1025
if (this->export_default_statement_span_.has_value()) {
this->diag_reporter_->report(
Diag_Multiple_Export_Defaults{
.second_export_default = this->peek().span(),
.first_export_default = *this->export_default_statement_span_,
});
} else {
this->export_default_statement_span_ = this->peek().span();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines 548 to 550
// If set, refers to the first `export default` statement in this module. A
// module cannot contain more than one `export default`.
std::optional<Source_Code_Span> export_default_statement_span_ = std::nullopt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.first_export_default = *this->export_default_statement_span_,
});
} else {
this->export_default_statement_span_ = this->peek().span();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think the name export_default_statement_span_ is wrong. The span is not for the whole statement, but for the 'default' keyword. first_export_default_statement_default_keyword_ is a good (but long) name. (I think length isn't a problem in this case; clarity is more important.)

@strager strager linked an issue Oct 25, 2023 that may be closed by this pull request
@arieldon arieldon force-pushed the multiple-export-defaults branch from 6d77c61 to 28dcac6 Compare October 25, 2023 14:31
@arieldon
Copy link
Contributor Author

I agree with your nitpicks, so I made the changes. I also fixed the error code in the header of the documentation file. It was E0714. Now it's E0715 as it should be. I ran tools/format too.

@strager
Copy link
Collaborator

strager commented Oct 25, 2023

Landed as commit 4bf1896. Thanks for the improvement!

@strager strager closed this Oct 25, 2023
@arieldon arieldon deleted the multiple-export-defaults branch October 28, 2023 00:12
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.

8$: error on multiple 'export default'-s
2 participants