Skip to content

Fix #226: Parse quoted blocks with spaces (' {) #227

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

Closed
wants to merge 1 commit into from

Conversation

lolgab
Copy link
Contributor

@lolgab lolgab commented Nov 11, 2021

Fixes the linked issue.

@lolgab lolgab force-pushed the fix-quoted-with-spaces branch from 69c8e35 to b3d1903 Compare November 11, 2021 22:57
@@ -678,11 +678,11 @@ export const scalaTmLanguage: TmLanguage = {
'scala-quoted': {
patterns: [
{ // Start of `'{ .. }` or `${ .. }`
match: "['$]\\{(?!')",
match: "['$][ \\t]*\\{(?!')",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used [ \\t] instead of \\b because the Scala compiler doesn't accept newlines here.

Copy link
Contributor

Choose a reason for hiding this comment

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

A single rule that captures the ' or $ but not rest.

{
  match: "['$](?=[ \\t]*[{[](?!'))"
  name: "..." // not sure what this should be
              // maybe "escape.scala" like in string interpolators
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could also take any space characters

  match: "['$](?=\\s*[{[](?!'))"

Copy link
Contributor

Choose a reason for hiding this comment

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

To be more precise

  match: "['$](?=[{[](?!')|\\s+[{[])"

as it is valid to have ${'{...}} even though a warning will be emitted

@@ -5,11 +5,21 @@
// ^ constant.numeric.scala
// ^ punctuation.section.block.end.scala

' { 2 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I added both a tab and a space to cover the handling of tab characters I have in the code.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good! We had the same issue recently in Scalameta parser.

@nicolasstucki
Copy link
Contributor

That does not seem right. I believe that the syntax requires the quote to be immediately followed by a curly brace. I will double check.

@nicolasstucki nicolasstucki self-assigned this Nov 12, 2021
@tgodzik
Copy link
Contributor

tgodzik commented Nov 12, 2021

That does not seem right. I believe that the syntax requires the quote to be immediately followed by a curly brace. I will double check.

It doesn't actually. At least last time I checked it did compile with whitespaces :O

@nicolasstucki
Copy link
Contributor

Indeed. I opened an issue to fix this in scala/scala3#13951.

For

' 	{ 2 }
' [ String ]

it looks like the current highlighting is correct for both Scala 2 and Scala 3 (after bug fix) as it is an unclosed Char or a Char with illegal contents.

@lolgab
Copy link
Contributor Author

lolgab commented Nov 17, 2021

Thank you @nicolasstucki for fixing the underlying issue :)

@lolgab lolgab closed this Nov 17, 2021
@tgodzik
Copy link
Contributor

tgodzik commented Dec 9, 2021

@nicolasstucki should we reopen this PR since it turns out it's a correct behaviour?

@lolgab
Copy link
Contributor Author

lolgab commented Dec 10, 2021

I'm reopening since the compiler is not changing behavior.
Still, this change is not perfect because it doesn't parse a comment in the middle of ' and {.

@lolgab lolgab reopened this Dec 10, 2021
@nicolasstucki
Copy link
Contributor

This is still missing some cases, see and other comments in that thread #227 (comment)

@nicolasstucki
Copy link
Contributor

This was fixed in #250

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