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

Conditional styling #365

Closed
jayhesselberth opened this issue Mar 3, 2018 · 20 comments · Fixed by #560
Closed

Conditional styling #365

jayhesselberth opened this issue Mar 3, 2018 · 20 comments · Fixed by #560

Comments

@jayhesselberth
Copy link

@jayhesselberth jayhesselberth commented Mar 3, 2018

It would be nice to be able to turn styling on and off for different sections of a file.

There are cases where the greedy behavior tries to "fix" intentional formatting. Consider this example, where many assignments are organized to emphasize the assignment value.

https://github.com/r-lib/pkgdown/blob/master/R/rd-html.R#L390

The nocov start/nocov end behavior of covr might be one approach. So styling of this:

x <-  'style me'

# nostyle start
x <-       "intentional indent"
y <-       "another one"
# nostyle end

would generate:

x <- "style me"

# nostyle start
x <-       "intentional indent"
y <-       "another one"
# nostyle end
@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Mar 3, 2018

Hi @jayhesselberth. I wonder if it is desirable to have source code with a lot of these comments in it. Personally, I think I would not like it that much. But there might be people who would find it useful.
As far as a potential implementation goes, I think this would be hard. With the nested parse structure, I don't see a straight forward way to do that at the moment. Do you see one?

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Mar 5, 2018

For your special use case, you can also use the option strict = FALSE to preserve multiple spaces after the assignment operator. See example section of style_text().

@jayhesselberth

This comment has been minimized.

Copy link
Author

@jayhesselberth jayhesselberth commented Mar 7, 2018

Quick stab at this. I may be off-base but once tokens are flagged then I think you just need to make the transformers aware of the style value (T/F).

library(tidyverse)
library(Rcpp)

nostyle <- Rcpp::cppFunction(
  'LogicalVector fn(DataFrame x) {

    CharacterVector text = x["text"] ;
    LogicalVector style(text.size()) ;
    int nostyle = 0;

    for (int i = 0; i < text.size(); i++) {
      if (text[i] == "# nostyle start") {
        nostyle++ ; 
        style[i] = false ; 
      } else if (text[i] == "# nostyle end") {
        nostyle-- ;
        style[i] = false ; 
      } else if (nostyle == 0) {
        style[i] = true ;
      } else {
        style[i] = false ;
      }
    }

    return style ;
  }' 
)

text <- c('
  x <- "text"
  # nostyle start
  y<-  "nostyle"
  # nostyle end
  z <- "text"
')

styler:::tokenize(text) %>%
  mutate(style = nostyle(.)) %>%
  select(token, text, style)
#> # A tibble: 20 x 3
#>    token       text            style
#>    <chr>       <chr>           <lgl>
#>  1 expr        ""              TRUE 
#>  2 SYMBOL      x               TRUE 
#>  3 expr        ""              TRUE 
#>  4 LEFT_ASSIGN <-              TRUE 
#>  5 STR_CONST   "\"text\""      TRUE 
#>  6 expr        ""              TRUE 
#>  7 COMMENT     # nostyle start FALSE
#>  8 expr        ""              FALSE
#>  9 SYMBOL      y               FALSE
#> 10 expr        ""              FALSE
#> 11 LEFT_ASSIGN <-              FALSE
#> 12 STR_CONST   "\"nostyle\""   FALSE
#> 13 expr        ""              FALSE
#> 14 COMMENT     # nostyle end   FALSE
#> 15 expr        ""              TRUE 
#> 16 SYMBOL      z               TRUE 
#> 17 expr        ""              TRUE 
#> 18 LEFT_ASSIGN <-              TRUE 
#> 19 STR_CONST   "\"text\""      TRUE 
#> 20 expr        ""              TRUE

Created on 2018-03-07 by the reprex package (v0.2.0).

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Mar 7, 2018

Thanks for looking into this. Yes, I was thinking about a flag too but this would require to adapt large parts of styler, making things more complicated and probably lead to significant slowdown for
something I don't consider to be worth.

An alternative approach would be to collapse all tokens between flags into a string (which would make it invisible for formatting in some sense), applying all rules and serialise. I guess that might be more pragmatic. You can even implement that yourself via transformer functions I think. Just put these transformer function first (see apply_transformers() for details on the order and structure of the transformers). If we want to do this, we just need to make sure that in the step of choose_indention(), you can add the correct absolute indention within the string (so it needed some infrastructure updates anyways). Should be doable since the parse table is already flattened out. I might have missed something though, so not 100% sure if it works out...

However, instead of implementing such functionality, I think I want to put my energy into changing styler so there are ways around flagging, i.e. by adapting rules such that they are flexible enough to accommodate edge cases. Have you given the strict = FALSE option a try? I know it's not perfect, but I think it can help in many cases. And as far as my personal handling goes: I also don't like everything styler does because there are edge cases, so after styling I just revert them manually.

Maybe @krlmlr can give his insights on the topic and whether it would be worth implementing conditional styling? If yes, maybe you could comment on my considerations.

@jayhesselberth

This comment has been minimized.

Copy link
Author

@jayhesselberth jayhesselberth commented Mar 8, 2018

Yes I think the strict = FALSE approach is fine for some cases but is only useful when selecting and styling a few lines of a file. I was thinking of the more robust approach where you can conditionally turn styling off for sections and then style the whole file/package.

The flattening approach sounds like it is worth exploring. I can take a look sometime soon.

This isn't urgent but struck me as something that would improve styler utility. I increasingly use the the nocov start/end strategy and it saves a lot of time hand-picking where to apply rules.

@krlmlr

This comment has been minimized.

Copy link
Member

@krlmlr krlmlr commented Mar 8, 2018

We have an issue for detecting alignment (and leaving it alone): #258. I agree with Lorenz that detecting and consistently styling aligned blocks of code is a better time investment. To me, the presence of markers contradicts the idea of improving code style.

Maybe we could support turning off styling for individual functions with a configuration file?

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Mar 23, 2018

I don't think turning off and on for individual functions is meeting the requirements layed out by @jayhesselberth. If we do it, it should be comprehensive.

@pat-s

This comment has been minimized.

Copy link
Contributor

@pat-s pat-s commented May 9, 2019

@lorenzwalthert
We are currently facing the situation in which we would like to maintain some spacing in a R6 dictionary like here.

This would probably only be possible by a "no-style" tag or similar as proposed in this issue? Or is there another workaround besides turning off all the spacing rules?

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented May 9, 2019

Have you tried strict = FALSE?

@pat-s

This comment has been minimized.

Copy link
Contributor

@pat-s pat-s commented May 10, 2019

This helps but also turns off other rules that we want to have applied. In the end strict only enables two versions of the style guide, right?. This is a start for conditional styling but does not really help if someone wants to handle edge cases (by just commenting these out from being styled).

I guess I've found the rule I was searching for now :)

#set_space_after_comma # old
add_space_after_comma # new
@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented May 10, 2019

We could modify that function to check if all other elements of the nest (that is, the context) are aligned are aligned and if so, not modify any spaces. So allignment would be preserved if it is already consistent. Not sure if it is as easy as it sounds though. Have to check.

@pat-s

This comment has been minimized.

Copy link
Contributor

@pat-s pat-s commented May 10, 2019

Sounds like a good idea! I guess this would solve the problem in this case.

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented May 11, 2019

Let's continue over at #258.

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Oct 8, 2019

Opened because of a request by @michaelquinn32.

@lorenzwalthert lorenzwalthert reopened this Oct 8, 2019
@michaelquinn32

This comment has been minimized.

Copy link

@michaelquinn32 michaelquinn32 commented Oct 8, 2019

Thanks!!!

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Oct 11, 2019

After giving it some more thought, I believe there could be a way to implement this feature without creating much complication and slow-down. Here is the proposal:

The user should be able to supply code with like this:

# stylerignore start
code = "like this"
# stylerignore stop

And later on also code # stylerignore.

Here is how to make sure this code does not get styled:

  • Before parse data is nested, we find the stylerignore tags and add a column
    stylerignore to the parse data which is false for all token except the
    tokens that fall between two stylerignore tags.

  • After code is turned into nested parse data and spaces and newlines are
    computed (must be between terminals, not between any token as currently done with
    default_style_guilde_attributes() edit: Because we need the position of tokens relative to other terminals, not tokens in general when serializing, we must do default_style_guilde_attributes() twice in total now). We store the lag_spaces and
    lag_newline attributes for all terminal tokens that are marked as
    stylerignore in the environment env_current.

  • We proceed as usual with appyling transformers and flattening out the parse
    table.

  • Right before we serialize the parse table, we replace lag_newlines, lag_spaces
    and tokens, text with the initial values for all tokens that were marked as
    stylerignore. Then we serialize.

We need to take care of:

  • Clearing env_current$stylerignore before every run.
  • Making the implementation as simple as possible and document it.
  • Ensuring no performance impact. E.g. check at beginning if there is any
    stylerignore tag anywhere (which should not be the case mostly) and then
    make evaluation of expensive functions dependent on it.
  • Which tags to use? stylerignorestart / stop?
  • Error handling if tags are not set correctly.

@krlmlr and others: what do you think? I believe this is much smarter than trying to check within every transformer / rule if a certain token should be modified.

I still don't like the feature but since this seems not so complicated to implement and a few people think it's really helpful, why not? Related tools such as lintr or black also have it.

@krlmlr

This comment has been minimized.

Copy link
Member

@krlmlr krlmlr commented Oct 11, 2019

I don't mind this feature, but I'm not sure I'd use it.

@michaelquinn32

This comment has been minimized.

Copy link

@michaelquinn32 michaelquinn32 commented Oct 11, 2019

This works for me, with two notes:

  • The standard Google format for these comments would be something like # styler: disable and # styler: enable
  • Per that, being able to customize the disabling comment (like in lintr) would be nice.

Thanks for working on this!

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Oct 12, 2019

Ok. I guess the comment format could be specified via an R option.

This was referenced Oct 16, 2019
@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Oct 22, 2019

We have a draft :-). Feedback welcome, please in the PR (#560), not here in the issue.

remotes::install_github("lorenzwalthert/styler@stylerignore")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.