Skip to content
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.

Sign up for GitHub

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

Jump to bottom

improved escaping sequences for indented strings in nix lexer #926

Merged
merged 4 commits into from
Jun 13, 2018
Merged

improved escaping sequences for indented strings in nix lexer #926

merged 4 commits into from
Jun 13, 2018

Conversation

veprbl
Copy link
Contributor

@veprbl veprbl commented Jun 6, 2018 •
edited
Loading

Fixes: #924

let
  hello = throw "this will not be used";
  double_quoted = " \n \r \q \" ''\n \\ ";
  mystr = "foo";
in
  assert "\q" == "q";
  assert ''''\q'' == "q";
  assert "$${hello}" == "$" + "$" + "{hello}";
  assert ''$${hello}'' == "$" + "$" + "{hello}";
  assert "$$${mystr}" == "$" + "$" + mystr;
  assert ''$$${mystr}'' == "$" + "$" + mystr;
  ''
    First, rouge is missing antiquotes escaping:
     ''${hello} should not terminate the string
     $${hello} is another (strange) way to escape

    Then, indented strings can be escaped as using triple single quote:
     ''' should not terminate the string

    Indented strings also have different set of escape sequences:
     \n is not a line feed and should not be highligted as an escape
     ''\n is a line feed, it should not terminate the string
       (pygments doesn't seem to know that)
     ''\q will evaluate to just 'q', it should not terminate the string
       (both nix-mode and pygments don't seem to know that)
  ''

image

veprbl added 3 commits June 6, 2018 18:22
@veprbl
nix lexer: s/string_double_quoted/double_quoted_string/, s/string_ind…
f17e10c 
…ented/indented_string/
@veprbl
nix lexer: improved escaping sequences support for indented strings
9493c74 
First rouge is missing antiquotes escaping:
 ''${hello} should not terminate the string

Then, indented strings can be escaped as using triple single quote:
 ''' should not terminate the string

Indented strings also have different set of escape sequences.
 \n is not a line feed and should not be highligted as an escape
 ''\n is a line feed, it should not terminate the string
   (pygments doesn't seem to know that)
 ''\q will evaluate to just 'q', it should not terminate the string
   (both nix-mode and pygments don't seem to know that)

There is also a passage in nix manual saying:
  "$ removes any special meaning from the following $"
I'm still not sure what it supposed to mean.
@veprbl
improved sample file for nix
0b15442 
 * use fancier example for antiquotation
 * "${x}" is not escaped actually
 * modernize configureFlags example
 * showcase escaping in indented string
@orivej
Copy link

orivej commented Jun 7, 2018 •
edited
Loading

This looks very accurate, but the lexer still seems not to know that ''$${1}'' is the same string as "$"+"$"+"{1}" or "$${1}", i.e. that $ escapes the next $ in strings.

@veprbl
Copy link
Contributor Author

veprbl commented Jun 7, 2018

@orivej This is an odd one. Thanks!

Not clear what purpose it serves in nixlang. Probably someone can hit it in the case when varname is a name of a bash variable and you have, say buildPhase = "echo $${varname}";, but not sure what problem it solves here. Good to handle it properly, though. Even github doesn't understand it currently.

@orivej
Copy link

orivej commented Jun 7, 2018

It is a quirk of Nix parser that was never fixed, but now Nixpkgs rely on it in several places.

@veprbl veprbl changed the title improved escaping sequences support for indented strings in nixlang lexer improved escaping sequences for indented strings in nix lexer Jun 9, 2018
@dblessing
Copy link
Collaborator

@veprbl Thanks for the PR. Looks good to me. Do we need to change anything wrt the conversation you and @orivej had above?

dblessing
dblessing approved these changes Jun 9, 2018
View reviewed changes
Copy link
Collaborator

@dblessing dblessing left a comment

Choose a reason for hiding this comment

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

Approve pending response on previous question. If we don't need to account for the issue mentioned in comments then we're good to go.

@dblessing dblessing added the needs-review The PR needs to be reviewed label Jun 9, 2018
@veprbl
Copy link
Contributor Author

veprbl commented Jun 9, 2018

I've addressed @orivej 's comment in 894c3a6 .

@dblessing dblessing added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 9, 2018
@veprbl
Copy link
Contributor Author

veprbl commented Jun 10, 2018

I think this is good to go. Not sure what changes are requested.

@dblessing
Copy link
Collaborator

Thanks @veprbl. You were just too fast in replying. I came back and did some categorization of PRs via labels and so the label came after your reply. Nothing else needed here. Thanks a lot for the contribution.

@dblessing dblessing merged commit 981e545 into rouge-ruby:master Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblessing dblessing dblessing approved these changes

@orivej orivej orivej approved these changes

Assignees
No one assigned
Labels
author-action The PR has been reviewed but action by the author is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@veprbl @orivej @dblessing