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

remove spaes after !! and !!! #322

Merged
merged 1 commit into from Feb 23, 2018
Merged

Conversation

lorenzwalthert
Copy link
Collaborator

Closes #321 but does even more because it also remove the space after!!!. It seems not so easy to distinguish !! and !!! since they are on separate levels in the nested parse table and we only have token_before and token_after, but we also needed token_before2 and token_after2 which does not seem to be particularly elegant. An alternative is go inside the child to see what comes afterwards, and set the spacing before processing the child. Also not very nice. Maybe let's remove the space after both !! and !!! for now.

styler:::create_tree("call(!!!a)")
#>                                                  levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {pos_id})
#> 2   °--expr:  [0/0] {1}                                   
#> 3       ¦--expr:  [0/0] {3}                               
#> 4       ¦   °--SYMBOL_FUNCTION_CALL: call [0/0] {2}       
#> 5       ¦--'(': ( [0/0] {4}                               
#> 6       ¦--expr:  [0/0] {5}                               
#> 7       ¦   ¦--'!': ! [0/0] {6}                           
#> 8       ¦   °--expr:  [0/0] {7}                           
#> 9       ¦       ¦--'!': ! [0/0] {8}                       
#> 10      ¦       °--expr:  [0/0] {9}                       
#> 11      ¦           ¦--'!': ! [0/0] {10}                  
#> 12      ¦           °--expr:  [0/0] {12}                  
#> 13      ¦               °--SYMBOL: a [0/0] {11}           
#> 14      °--')': ) [0/0] {13}

Created on 2018-01-14 by the reprex package (v0.1.1.9000).

@codecov-io
Copy link

codecov-io commented Jan 14, 2018

Codecov Report

Merging #322 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #322   +/-   ##
======================================
  Coverage    91.2%   91.2%           
======================================
  Files          30      30           
  Lines        1376    1376           
======================================
  Hits         1255    1255           
  Misses        121     121
Impacted Files Coverage Δ
R/rules-spacing.R 96.07% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d71bc2...7045a29. Read the comment docs.

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Feb 23, 2018

@krlmlr should aim to alter the parse data before nesting it and make !! and !!! one token? That would allow us to handle !! and !!! differently (or not touch !!!). Not sure whether it's worth the effort.

@lorenzwalthert
Copy link
Collaborator Author

Reference: tidyverse/dplyr#3299 (comment)

@lorenzwalthert
Copy link
Collaborator Author

Reference: tidyverse/style#55

@krlmlr
Copy link
Member

krlmlr commented Feb 23, 2018

I could live with looking a few levels into the parse tree, e.g.: pd$child[[2]]$child[[2]]$token == "!" . For now, let's get rid of spaces after !!! for now, we can add the distinction later if it's specified in tidystyle.

@lorenzwalthert
Copy link
Collaborator Author

Yes, looking into the child is one thing, but manipulating it is even more questionable. Ok, sure.

@lorenzwalthert lorenzwalthert merged commit 5982e83 into r-lib:master Feb 23, 2018
@lorenzwalthert lorenzwalthert deleted the bang branch March 2, 2018 13:39
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.

None yet

3 participants