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

Don't force the first switch argument in-line with switch( #858

Open
MichaelChirico opened this issue Oct 25, 2021 · 8 comments
Open

Don't force the first switch argument in-line with switch( #858

MichaelChirico opened this issue Oct 25, 2021 · 8 comments

Comments

@MichaelChirico
Copy link
Contributor

I think we've hewed too closely to the style guide after #714:

library(styler)

styler::style_text('
switch(
  x,
  a = 1,
  b = 2,
  c = 3
)')
# switch(x,
#   a = 1,
#   b = 2,
#   c = 3
# )

Understand the switch() example in the style guide is like that, but I think both having x on its own line and moving it to switch(x are acceptable per the overall style guide, so styler shouldn't override valid syntax.

My reasoning is per: https://style.tidyverse.org/syntax.html#long-lines

As described under Named arguments, you can omit the argument names for very common arguments (i.e. for arguments that are used in almost every invocation of the function). Short unnamed arguments can also go on the same line as the function name, even if the whole function call spans multiple lines.

This becomes worse when the first argument is an expression, not just a symbol:

library(styler)

styler::style_text('
switch(
  tolower(x$switch_column),
  a = 1,
  b = 2,
  c = 3
)')
# switch(tolower(x$switch_column),
#   a = 1,
#   b = 2,
#   c = 3
# )

Here I think the original version is much clearer.

@lorenzwalthert
Copy link
Collaborator

I am happy to keep it if on a separate line. Cn you file a PR?

@MichaelChirico
Copy link
Contributor Author

Sure, I will give it a go. It will take a while to get to it.

@lorenzwalthert
Copy link
Collaborator

Let me know how I can help. Most likely the diff for #714 is a good starting point.

@MichaelChirico
Copy link
Contributor Author

It also seems like styler insists switch() be split across multiple lines, even when it fits cleanly in just one, e.g.:

styler::style_text("switch(tolower(suffix), k = 1, m = 2, g = 3, t = 4)")
# switch(tolower(suffix),
#   k = 1,
#   m = 2,
#   g = 3,
#   t = 4
# )

@lorenzwalthert
Copy link
Collaborator

One liners are explicitly discouraged in the type guide, see #714 (comment).

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Oct 27, 2021

Thanks, that's unfortunate, it really is a huge waste of space for cases when it can fit inline. I'll file upstream -- the one-line examples in the guide all use fall-through, where I can see the case for splitting it out more easily.

@lorenzwalthert
Copy link
Collaborator

@MichaelChirico can you add the reference to the issue in https://github.com/tidyverse/style here?

@MichaelChirico
Copy link
Contributor Author

That's tidyverse/style#181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants