Skip to content

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.reporting.diagnostic.messages.IdentifierExpected
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