Skip to content

Overriding tidyverse function arg style #1184

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
JosiahParry opened this issue Mar 27, 2024 · 23 comments
Closed

Overriding tidyverse function arg style #1184

JosiahParry opened this issue Mar 27, 2024 · 23 comments

Comments

@JosiahParry
Copy link

I'd pose this in a discussion but there are not any for this repo!

The tidyverse style guide says:

In both cases the trailing ) and leading { should go on the same line as the last argument.

However, it the closing ) and opening { on the same line as the last argument give me the ick! 😜 Is there a way to modify styler to put the closing paren and opening bracket on their own line when there are many function arguments?

I use codegrip to format my function arguments. However, I have recently configured my environment to use styler to format on save. This one styling, though, i'd like to override.

reverse_geocode <- function(
    locations,
    crs = sf::st_crs(locations),
    ...,
    lang_code = NULL,
    feature_type = NULL,
    for_storage = FALSE,
    location_type = c("rooftop", "street"),
    preferred_label_values = c("postalCity", "localCity"),
    geocoder = default_geocoder(),
    token = arc_token(),
    .progress = TRUE
) {
@IndrajeetPatil
Copy link
Collaborator

I agree that this should be the default, but, unfortunately, since the underlying style guide specifies this behaviour, we need to stick to it.

As for codegrip, there is a PR to bring it in line with the tidyverse style guide, so it might also start behaving the same way as {styler}.

That said, {grkstyle} defaults to the behaviour you desire, so you might want to check it out.

@JosiahParry
Copy link
Author

JosiahParry commented Mar 27, 2024

Gotya, so are you saying that styler is so opinionated that it will not permit modification outside of the tidyverse style guide?

Edit: grkstyle does not adhere to balanced delimiters

@IndrajeetPatil
Copy link
Collaborator

It's not {styler} that is opinionated, it's the style guide 🙃

But {styler} does allow further customizations: https://styler.r-lib.org/articles/customizing_styler.html

TBH, I'd much rather the style guide change its recommendation here.

@JosiahParry
Copy link
Author

{styler} is enforcing the style guide, thus opinionated! 🙈
Are the stylers documented? It's tough to figure out which one it might be.

Say I've figured out how to customize my styler, how do I utilize it as a global standard? For example with lintr, there is a .lintr file. The customizing-style, third party integration, and distribute custom styles vignettes do not describe how to make a custom styler the default when working with styler.

For context, I'm using VS Code to format on save and not manually styling a file from the R console. Ideally it can be modified in the Rprofile or other environment location

@JosiahParry
Copy link
Author

I believe this may be possible in r-lib/lintr#1411 by @MichaelChirico but unclear how 🤔

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Mar 27, 2024

The customizing-style, third party integration, and distribute custom styles vignettes do not describe how to make a custom styler the default when working with styler.

Two options:

  1. You just call {styler} with specifying transformers argument with the style guide you export from your package.

  2. You don't use {styler} directly anymore if you create your own style guide based on the styler infra, you just call your own pkg where style_file() and friends have a different default for transformers . E.g. see this README.

library(styler.noncomments)
style_text('1 # comment') # this default to transformers = nocomments_style

@lorenzwalthert
Copy link
Collaborator

a directory specific configuration for styler is an open issue: #319

@JosiahParry
Copy link
Author

@lorenzwalthert I have no intention of creating and distributing my own R package for modifying a single style rule. It would ideal to be able to change or remove one rule in the transformers list and have that be used. Perhaps it can be set via options() in a user-level .Rprofile?

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Mar 27, 2024

For VS code specifically, see https://github.com/REditorSupport/languageserver?tab=readme-ov-file#customizing-formatting-style.

You can modify a rule inline, e.g. in the above mentioned link, but I think for caching reasons, you should also set name and version of the style guide to avoid caching issues as described in #1170 (comment).

  ...
  transformers$style_guide_name = "rpolars_style"
  transformers$style_guide_version = "0.1.0"
  ...

@lorenzwalthert
Copy link
Collaborator

I think it seems like the style guide might change in the future or at least allow both options since Lionel supports your point of view in https://github.com/lionel-/codegrip/pull/16, but for this to happen, I think as @IndrajeetPatil said, it needs a formal decision upstream. Depending on what happens there, we could change the behaviour or expose an argument in tidyverse_style() to configure the behaviour.

@lorenzwalthert
Copy link
Collaborator

@lorenzwalthert I have no intention of creating and distributing my own R package for modifying a single style rule.

Then I am afraid you have to adhere to the style guide, or open an issue in tidyverse/style.

@JosiahParry
Copy link
Author

Heard.

Just to be crystal clear, the maintainers of {styler} have no intention of making it possible to simply modify styler similar to lintr's ability to modify them via options e.g.

options(lintr.line_length_lintr = NULL)

and instead, require users to create their own R package for this behavior?

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Mar 27, 2024

I might be able to show you how you can modify styler to achieve what you want, but unless you create your own style guide, the option won’t be as portable as a simple one liner in a config file like for {lintr}.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Mar 27, 2024

Intentions we have to make configuration

  1. of the style guide function to use
  2. Of the arguments to pass to the style guide

easier to access through a config file or similar as noted in #319.

But time and resources and priorities are a different topic 😜

To achieve what you wanted in your original post, we would first have to expose an argument to the default style guide tidverse_style to allow configuring styler in a way that the style guide currently clearly states what the compliant style looks like, leaving everything else non-compliant.

@JosiahParry
Copy link
Author

Closing in favor of #319

@lionel-
Copy link
Member

lionel- commented Mar 28, 2024

FWIW I think we'd be supportive of a style guide PR to bring it in line with codegrip behaviour. We've originally aligned our guide to the google C++ one, but after working with Rust and Typescript where the balanced style seem to be the norm we think it's best to align with these languages.

@JosiahParry
Copy link
Author

This is now official tidyverse style see https://style.tidyverse.org/functions.html#multi-line-function-definitions

Can we consider reopening this issue?

@IndrajeetPatil
Copy link
Collaborator

With the latest commit on the main-branch:

"reverse_geocode <- function(
  locations,
  .progress = TRUE) {
  NULL
}" -> code

styler::style_text(code)
#> reverse_geocode <- function(
#>   locations,
#>   .progress = TRUE
#> ) {
#>   NULL
#> }

Created on 2024-11-25 with reprex v2.1.1

@JosiahParry
Copy link
Author

Nice!! For a second I though I got a bug report in my email! 🫣

@lorenzwalthert
Copy link
Collaborator

I guess we can close this now.

@IndrajeetPatil
Copy link
Collaborator

@lorenzwalthert Should we create a new CRAN release?

I would say that enough has changed to make it available to the users.

@lorenzwalthert
Copy link
Collaborator

I think we should also implement some of the other open issues that resulted from the style guide update before that.

@IndrajeetPatil
Copy link
Collaborator

@lorenzwalthert So only after closing this issue?

#1231

One of those items is open upstream, so it might be a while before that issue can be closed. But maybe we can try to address the other two items in it and prepare a release.

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

No branches or pull requests

4 participants