Skip to content

Conversation

@john-eevee
Copy link
Contributor

@john-eevee john-eevee commented Oct 30, 2018

This PR removes unnecessary backticks ` from messages and explanations, around interpolations and from text, wraping those texts into interpolations so it can be picked by the highlighter

  • `()` becomes ${"()"}
  • `$mySymbol` becomes $mySymbol

Also capitalizes messages, but:

  • Messages that starts with keywords such as match were not capitalized.
  • Messages starting with symbol name such as value, class, object were not capitalized.

This all to remove clutter and increase experience when sending those errors to editors for example, and dont let newcomers be confused since ` is part of the language when used in a case expression and to escape keywords

Part of #1589

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@john-eevee
Copy link
Contributor Author

john-eevee commented Oct 30, 2018

Waiting on tests to perform on CI so I can fix any broken, since on my machine I can't compile the master branch

Fix test resources by making them
be like the result from messages

Remove backticks from 1379
@john-eevee
Copy link
Contributor Author

Tests fixed!

@nicolasstucki nicolasstucki self-assigned this Dec 10, 2018
@nicolasstucki
Copy link
Contributor

@sleepiejohn could you rebase your PR the changes so far look good.

| found: Int(1)
| required: String
| Found: Int(1)
| Required: Stri
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed part of String by mistake 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.

Oh dang, my bad!

@john-eevee
Copy link
Contributor Author

Seems that language server tests are now included and it's currently broke because of capitalization, I'll tend to it later today

Fix LS diagnostics tests
@nicolasstucki nicolasstucki merged commit 07d75ce into scala:master Dec 12, 2018
@nicolasstucki
Copy link
Contributor

Thanks @sleepiejohn

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