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.

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

Add new R pipe support #127

Merged
merged 12 commits into from
Jun 11, 2022
Merged

Add new R pipe support #127

merged 12 commits into from
Jun 11, 2022

Conversation

cderv
Copy link
Contributor

@cderv cderv commented Feb 1, 2022

This PR closes #126

Parsing shows that token are PIPE and PIPEBIND

Sys.setenv("_R_USE_PIPEBIND_"= TRUE)
downlit:::parse_data("1 |> x => fun(2, x)")
#> $text
#> [1] "1 |> x => fun(2, x)"
#> 
#> $expr
#> expression(1 |> x => fun(2, x))
#> 
#> $data
#>    line1 col1 line2 col2 id parent                token terminal text
#> 25     1    1     1   19 25      0                 expr    FALSE     
#> 1      1    1     1    1  1      2            NUM_CONST     TRUE    1
#> 2      1    1     1    1  2     25                 expr    FALSE     
#> 3      1    3     1    4  3     25                 PIPE     TRUE   |>
#> 24     1    6     1   19 24     25                 expr    FALSE     
#> 4      1    6     1    6  4      6               SYMBOL     TRUE    x
#> 6      1    6     1    6  6     24                 expr    FALSE     
#> 5      1    8     1    9  5     24             PIPEBIND     TRUE   =>
#> 22     1   11     1   19 22     24                 expr    FALSE     
#> 7      1   11     1   13  7      9 SYMBOL_FUNCTION_CALL     TRUE  fun
#> 9      1   11     1   13  9     22                 expr    FALSE     
#> 8      1   14     1   14  8     22                  '('     TRUE    (
#> 10     1   15     1   15 10     11            NUM_CONST     TRUE    2
#> 11     1   15     1   15 11     22                 expr    FALSE     
#> 12     1   16     1   16 12     22                  ','     TRUE    ,
#> 16     1   18     1   18 16     18               SYMBOL     TRUE    x
#> 18     1   18     1   18 18     22                 expr    FALSE     
#> 17     1   19     1   19 17     22                  ')'     TRUE    )

I have put them in the infix category with the magrittr pipe.

For new placeholder we have PLACEHOLDER token

downlit:::parse_data("1:10 |> mean(x = _)")
#> $text
#> [1] "1:10 |> mean(x = _)"
#> 
#> $expr
#> expression(1:10 |> mean(x = _))
#> 
#> $data
#>    line1 col1 line2 col2 id parent                token terminal text
#> 21     1    1     1   19 21      0                 expr    FALSE     
#> 7      1    1     1    4  7     21                 expr    FALSE     
#> 1      1    1     1    1  1      2            NUM_CONST     TRUE    1
#> 2      1    1     1    1  2      7                 expr    FALSE     
#> 3      1    2     1    2  3      7                  ':'     TRUE    :
#> 4      1    3     1    4  4      5            NUM_CONST     TRUE   10
#> 5      1    3     1    4  5      7                 expr    FALSE     
#> 6      1    6     1    7  6     21                 PIPE     TRUE   |>
#> 19     1    9     1   19 19     21                 expr    FALSE     
#> 8      1    9     1   12  8     10 SYMBOL_FUNCTION_CALL     TRUE mean
#> 10     1    9     1   12 10     19                 expr    FALSE     
#> 9      1   13     1   13  9     19                  '('     TRUE    (
#> 11     1   14     1   14 11     19           SYMBOL_SUB     TRUE    x
#> 12     1   16     1   16 12     19               EQ_SUB     TRUE    =
#> 13     1   18     1   18 13     14          PLACEHOLDER     TRUE    _
#> 14     1   18     1   18 14     19                 expr    FALSE     
#> 15     1   19     1   19 15     19                  ')'     TRUE    )

Not sure which highlight class it should have. Proposal in operator too for this one.

Added tests dependent on R requirement. Snapshots seems ok to use here.

tests/testthat/test-highlight.R Outdated Show resolved Hide resolved
@cderv

This comment was marked as resolved.

R/highlight.R Outdated
"'$'", "'@'","'~'", "'?'", "':'", "SPECIAL"
"'$'", "'@'","'~'", "'?'", "':'", "SPECIAL",
# pipes
"PIPE", "PIPEBIND", "PLACEHOLDER"
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, are you sure PLACEHOLDER should be treated as infix? I would think it should maybe just go in special?

Copy link
Contributor Author

@cderv cderv Jun 10, 2022

Choose a reason for hiding this comment

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

This _ felt more like an operator to me (with +, -, <, > ...) than a special like function(), switch() or other.
Among KDE default style, dsOperator seemed the closest. dsConstant was my second choice but it is supposed to be Number related.

However, I am ok to apply dsKeyword - I don't mind changing.

However, out of curiosity, I compared with other languages which have placeholder and it seems they are not highlighted specifically.

```scala
val f = _ + _ 
```
<div class="sourceCode" id="cb1">
<pre class="sourceCode scala"><code class="sourceCode scala">
<span id="cb1-1"><a href="#cb1-1" aria-hidden="true" tabindex="-1"></a>
<span class="kw">val</span> f <span class="op">=</span> _ <span class="op">+</span> _ 
</span>
</code></pre>
</div>

and

```python 
for _ in range(10):
	print "hello"
```
<div class="sourceCode" id="cb1">
<pre class="sourceCode python"><code class="sourceCode python">
<span id="cb1-1"><a href="#cb1-1" aria-hidden="true" tabindex="-1"></a>
<span class="cf">for</span> _ <span class="kw">in</span> <span class="bu">range</span>(<span class="dv">10</span>):</span>
<span id="cb1-2"><a href="#cb1-2" aria-hidden="true" tabindex="-1"></a>   <span class="bu">print</span> <span class="st">&quot;hello&quot;</span>
</span>
</code></pre>
</div>

Does not seem like current KDE file does a special treatment for this _ as placeholder character.
Highlight JS also does nothing special for the above snipper.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm maybe we should just treat it as a SYMBOL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like

"SYMBOL" = "va",

meaning dsVariable as variable name ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that. It looks ok to have the same highlighting as variables
image

tests/testthat/test-highlight.R Outdated Show resolved Hide resolved
"'$'", "'@'","'~'", "'?'", "':'", "SPECIAL"
"'$'", "'@'","'~'", "'?'", "':'", "SPECIAL",
# pipes
"PIPE", "PIPEBIND"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant you should do the transformation in this function, so we only need to do it in one place.

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 ok. I was under the impression you did associate some token classes from R parsed data to some chroma or pandoc classes directly. So I went with that for this one too as token_type() seemed to handle special grouping of some token type in infix, special, constant and parens groups.

I'll adapt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the change, hopefully I got it right. I did not went with that originally because you went with separate attributions for other types by duplicating

downlit/R/highlight.R

Lines 228 to 230 in 8434f36

"SLOT" = "va",
"SYMBOL" = "va",
"SYMBOL_FORMALS" = "va",

downlit/R/highlight.R

Lines 254 to 256 in 8434f36

"SLOT" = "nv",
"SYMBOL" = "nv",
"SYMBOL_FORMALS" = "nv",

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, that's reasonable read — I think this is different because PLACEHOLDER is only a single token so it makes more sense to lump it into one of the other classes.

@hadley hadley merged commit 36c7e5d into r-lib:main Jun 11, 2022
@cderv cderv deleted the new-pipe-support branch June 13, 2022 12:55
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.

Support R native pipe
3 participants