Skip to content

Fix case class/object highlighting #14

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
Mar 17, 2019
Merged

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