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

Added warning when elements have keyword names #155

Merged
merged 2 commits into from
Oct 20, 2018

Conversation

ivanbakel
Copy link
Contributor

Fixes #91.
To avoid erroneously preventing elements which are actually meant to be
called if from being used, this is only a warning, with a suggestion
of the template syntax.

Fixes lambda-fairy#91.
To avoid erroneously preventing elements which are actually meant to be
called `if` from being used, this is only a warning, with a suggestion
of the template syntax.
@ivanbakel
Copy link
Contributor Author

I'd like to add a test for this, but I couldn't find anything on testing Diagnostic emission.

match ident_string.as_str() {
"if" | "while" | "for" | "match" | "let" => {
ident.span()
.warning(format!("found keyword `{}` - should this be a `@{}`?", ident_string, ident_string))
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to use the same argument twice? Something like

format!("found keyword `{0}` - should this be a `@{0}`?", ident_string)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was.

@lambda-fairy
Copy link
Owner

I'd like to add a test for this, but I couldn't find anything on testing Diagnostic emission.

Yeah, we'll need to use compiletest-rs to test that properly.

I think it's okay to skip the test for now.

The formatting argument is now only passed once.
@ivanbakel
Copy link
Contributor Author

I had some time to get back to this, so I implemented a compiler test for the warning output. It's on a branch off this current one, if you don't want to merge it in as well.

Copy link
Owner

@lambda-fairy lambda-fairy left a 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, thanks!

I was going to wait for lint-associated warnings to land first (see rust-lang/rust#54140), so that users would have a way to silence this warning. But that probably doesn't matter. Let's push this out and see if anyone complains 😉

Great that you got compiletest working. Feel free to post another PR for that branch!

@lambda-fairy lambda-fairy merged commit 61e5f9d into lambda-fairy:master Oct 20, 2018
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.

2 participants