Skip to content

Update error message at Parsers.scala:1901 #1616

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
merged 1 commit into from
Oct 24, 2016

Conversation

ljdelight
Copy link
Contributor

@ljdelight ljdelight commented Oct 20, 2016

This one is about a 'missing return type' when we can't infer a type. I used tests/neg/i871.scala to verify the change

@ljdelight
Copy link
Contributor Author

FYI I'm behind at least one PR that will conflict. Noting this since I'm not familiar with the capabilities of the CI

|trait Shape {
| def area: Double
|}""".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 first off, you're correctly stating that if a trait method does not have a return type it needs to be clarified. But why is this? It's an abstract definition; we need to know what it's type is when we implement it somewhere else.

I also have a different nitpick, so the error messages obey a flag called -pagewidth X, where X is the max number of columns that should be printed in the terminal. As such, I would suggest that you split line 408 to be no more than 80 characters (i.e. newline after area).

Good job otherwise, and thanks for cleaning up the trailing whitespace ❤️

Copy link
Contributor Author

@ljdelight ljdelight Oct 21, 2016

Choose a reason for hiding this comment

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

Thanks for the comments. How does this read?

An abstract declaration must have a return type. For example:
        |
        |trait Shape {
        |  def area: Double // abstract declaration returning a ${"Double"}
        |}

Copy link
Contributor

@felixmulder felixmulder Oct 21, 2016

Choose a reason for hiding this comment

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

Yes, that looks fine. Make the change and this is approved for merge :)

@felixmulder
Copy link
Contributor

Oh and also, second nitpick!

Make your commit message subject line a bit shorter, put the rest in the body - example:

Update error message for missing return type

This one is about...
# Please enter the commit message for your changes...

Cheers!

@ljdelight ljdelight force-pushed the errorMessageUpdates branch from f20b48a to 7c99ecd Compare October 22, 2016 22:56
@ljdelight ljdelight changed the title Update error message at Parsers.scala:1901. This one is about a 'miss… Update error message at Parsers.scala:1901 Oct 22, 2016
@ljdelight ljdelight force-pushed the errorMessageUpdates branch from 7c99ecd to 7b774b0 Compare October 23, 2016 02:52
@felixmulder
Copy link
Contributor

LGTM - a rebase and an update on the errorId and we're good to go :)

This one is about a 'missing return type' when we can't infer a type. I used tests/neg/i871.scala to verify the change.
@ljdelight ljdelight force-pushed the errorMessageUpdates branch from 7b774b0 to c8595ca Compare October 23, 2016 14:00
@felixmulder felixmulder merged commit 56731e4 into scala:master Oct 24, 2016
@ljdelight ljdelight deleted the errorMessageUpdates branch October 24, 2016 20:52
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.

3 participants