-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add error message IdentifierExpected #1633
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
Add error message IdentifierExpected #1633
Conversation
7c52ddb
to
b3cc5f6
Compare
@felixmulder take a look when you have some time please =) |
import dotty.tools.dotc.util.SourceFile | ||
import util.Positions._ | ||
|
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.
Why the newline here?
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.
no need indeed...
| | ||
|""".stripMargin | ||
} | ||
} |
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.
So I have a couple of comments/questions on this explanation:
- Does it make sense for all cases?
- Could you show the user their code and the correct way to rewrite it? E.g:
def foo: int = { ... }
=>def foo = { ... }
- There are some weird things when it comes to the language like
"Because of Scala's stronger type"
- wat? Have a look through it and I'm sure you'll find places to correct :) - The line breaking is increasing, first line is something like 60 chars, the second 65 and third 90+, try to even it out.
- The line with "Here we explicitly" is too long (84 chars). We want a line to be max 80 chars (the standard terminal width).
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.
Hi @felixmulder thanks for your comments. I'm going through your cases and explain to you what I thought right?
1 - Actually, to be honest, the only piece of code I could reproduce this error was this:
def foo: this = "bar"
and it goes to the Parsers
class.
Is there any other case I could reproduce it, so I can make sure nothing was left behind?
2 - Get the user code, you mean I could use the Tree
to show the user wrong code and give the correct one? In fact I could pass the Tree
inside the class and use it. Don't know how to play with Tree
s but I will figure out.
3 - yeah, that one was difficult to write. I mean, Scala has a strong type system, which is good, everything has a type where it could be inferred by the compiler or not(in case the user provided it).
4 & 5 - No problem :)
655782d
to
8406d75
Compare
Hi @felixmulder take a look on the changes I have done. This time, minimal changes have been applied, no new lines, no import messing, nothing but the required code :) |
8406d75
to
2bdb3a2
Compare
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.
Minor changes, then we're done :)
| | ||
|$wrongIdentifier | ||
| | ||
|Write your sentence to: |
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.
"sentence to" should be "code like"
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.
ahhh of course, we are not talking about essay :)
val validIdentifier = s"def foo = {...}" | ||
|
||
val explanation = { | ||
hl"""|A valid identifier is expected, but $identifier was found. |
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.
maybe quotes around $identifier
so the line becomes:
hl"""|A valid identifier expected, but
$identifier found.
This commit adds the semantic object fir the ```identifier expected``` error. It is part of the scala#1589
2bdb3a2
to
1c0e48f
Compare
@felixmulder all set, including merge. |
Great - merging as soon as CI passes :) |
@felixmulder thank you! I will be looking for other issues to help :) |
This commit adds the semantic object fir the
identifier expected
error.It is part of the #1589
This PR fixes: