Skip to content

Conversation

@Hydrotoast
Copy link
Contributor

Hi maintainers,

Problem. The case keyword is assigned the scope keyword.control.flow.scala in the declarations case class and case object. This seems incorrect. See the image below.

screenshot-before

Proposal. After investigating, I discovered that the corresponding pattern did not account for whitespace between the case keyword and the class/object keywords. To fix this, we propose two changes in this PR:

  • matched potential whitespace between case and class/object declarations and
  • extracted an independent pattern for the trait declarations because they cannot be preceded by a case keyword.

The image below shows the result after these changes have been applied.

screenshot-after

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @Hydrotoast ! I agree that the current syntax highlighting for case class and case object is not good, and your screenshot looks much better 👍 Nice job tracking down the necessary changes in the syntax definition!

LGTM 👍 wdyt @nicolasstucki ?

@nicolasstucki
Copy link
Contributor

What happens with match/case? Does this also affect that colouring.

@nicolasstucki
Copy link
Contributor

Also how do other class modifiers look? Such as private, protected, ...

@Hydrotoast
Copy link
Contributor Author

Thanks for looking at the PR, @olafurpg and @nicolasstucki !

@nicolasstucki As requested, I have attached a screenshot with

  • several variations of class/object-level modifiers and
  • a match-case control flow expression nested within a case class.

class-mod

Note 1. In the screenshots, the color theme is "Dark +", which is provided by default. Generally, I think the colors may be misleading. For example, if we had a color scheme that assigned both scopes keyword.control.flow.scala and keyword.declaration.scala to the same color, then this issue may have been overlooked!

From the original code, it seemed clear that the case in case class was intended to be assigned the scope keyword.declaration.scala rather than keyword.control.flow.scala.

Note 2 (Out of scope). I am familiar with developing VS Code plugins or TextMate grammars: is there a generally accepted way to test changes to the TextMate grammars? I think automated tests may help prevent potentially undesirable syntax highlighting.

@nicolasstucki nicolasstucki merged commit 1c91eca into scala:master Mar 17, 2019
@nicolasstucki
Copy link
Contributor

Thank @Hydrotoast

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