Skip to content

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

Merged

Conversation

iamthiago
Copy link
Contributor

@iamthiago iamthiago commented Oct 26, 2016

This commit adds the semantic object fir the identifier expected error.
It is part of the #1589

This PR fixes:

  • parsing/JavaParsers.scala:233
  • Parsers.scala:319

@iamthiago iamthiago force-pushed the feature/error-identifier-expected branch from 7c52ddb to b3cc5f6 Compare October 26, 2016 22:23
@iamthiago
Copy link
Contributor Author

@felixmulder take a look when you have some time please =)

import dotty.tools.dotc.util.SourceFile
import util.Positions._

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the newline here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need indeed...

|
|""".stripMargin
}
}
Copy link
Contributor

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

Copy link
Contributor Author

@iamthiago iamthiago Oct 28, 2016

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 Trees 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 :)

@felixmulder felixmulder added the area:reporting Error reporting including formatting, implicit suggestions, etc label Oct 28, 2016
@iamthiago iamthiago force-pushed the feature/error-identifier-expected branch 3 times, most recently from 655782d to 8406d75 Compare October 29, 2016 13:57
@iamthiago
Copy link
Contributor Author

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 :)
Also I tried to be simple with the explanation. Take a look if that satisfies.
Thanks for your time :)
Cheers!

@iamthiago iamthiago force-pushed the feature/error-identifier-expected branch from 8406d75 to 2bdb3a2 Compare October 30, 2016 11:01
Copy link
Contributor

@felixmulder felixmulder left a 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:
Copy link
Contributor

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"

Copy link
Contributor Author

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.
Copy link
Contributor

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
@iamthiago iamthiago force-pushed the feature/error-identifier-expected branch from 2bdb3a2 to 1c0e48f Compare October 31, 2016 12:43
@iamthiago
Copy link
Contributor Author

@felixmulder all set, including merge.

@felixmulder
Copy link
Contributor

Great - merging as soon as CI passes :)

@felixmulder felixmulder merged commit b4f0c6e into scala:master Oct 31, 2016
@iamthiago iamthiago deleted the feature/error-identifier-expected branch October 31, 2016 13:39
@iamthiago
Copy link
Contributor Author

@felixmulder thank you! I will be looking for other issues to help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:reporting Error reporting including formatting, implicit suggestions, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants