-
-
Notifications
You must be signed in to change notification settings - Fork 808
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
Improve error message for invalid target in attribute #4369
Improve error message for invalid target in attribute #4369
Conversation
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.
Thank you!! Would you mind updating the changelog also? 🙏
compiler-core/src/parse.rs
Outdated
let (start, t, end) = match self.next_tok() { | ||
Some(t) => t, | ||
None => { | ||
return parse_error(ParseErrorType::UnexpectedEof, SrcSpan { start: 0, end: 0 }); | ||
} | ||
}; | ||
let location = SrcSpan::new(start, end); |
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.
Only build this when it is needed please 🙏
CHANGELOG.md
Outdated
@@ -222,6 +222,10 @@ | |||
- Fixed a bug where variant inference wouldn't work on `let assert` assignments. | |||
([Giacomo Cavalieri](https://github.com/giacomocavalieri)) | |||
|
|||
- Fixed a bug where the error message for an unknown or missing target name |
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.
This isn't a bug!
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.
How would you classify it? It seems like undesired behaviour to me.
Should I move it to the compiler section instead?
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.
This is an improvement to compiler error messages, there was nothing not-working previously so it's not a bug fix.
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.
Fair point👍
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.
Btw, thanks a lot for reviewing everything so quickly and in-depth.
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.
Thank you!
Resolves Issue #4362.
UnknownTarget
error in case the target name is neitherjavascript
norerlang
ExpectedTargetName
error in case the target is missing