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

brace_linter() could allow skipping braces in function factories #1807

Open
AshesITR opened this issue Dec 7, 2022 · 10 comments · May be fixed by #2240
Open

brace_linter() could allow skipping braces in function factories #1807

AshesITR opened this issue Dec 7, 2022 · 10 comments · May be fixed by #2240
Labels
feature a feature request or enhancement

Comments

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 7, 2022

lintr::lint(text = "function() function(x) {\n  x\n}")
#> <text>:1:1: style: [brace_linter] Any function spanning multiple lines should use curly braces.
#> function() function(x) {
#> ^~~~~~~~~~~~~~~~~~~~~~~~

Created on 2022-12-07 with reprex v2.0.2

This could optionally be allowed in addition to

# always allowed
function() {
  function(x) {
    x
  }
}

# could optionally be allowed
function() function(x) {
  x
}
@AshesITR AshesITR added the feature a feature request or enhancement label Dec 7, 2022
@MichaelChirico
Copy link
Collaborator

WDYT about making it the same option as allowing e.g.

function(x) AWrapper(
  y,
  x
)

?

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 7, 2022

Wouldn't that disable that particular lint completely?

@MichaelChirico
Copy link
Collaborator

No, there's still

function(x)
  AWrapper(y, x)

function(x)
  AWrapper(
    y,
    x
  )

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 7, 2022

Ah okay. Any ideas for an option name? allow_simple_multiline = FALSE is what comes to my mind, but I'm not really happy with it.

@IndrajeetPatil
Copy link
Collaborator

But omitting braces here reduces readability, since the reader now needs to mentally add those braces back in.

function() function(x) {
  x
}

Why should we make an exception for such an omission? That is, why is it so important here to leave out braces?

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 7, 2022

Because you need two additional lines and an additional level of indent.
Not arguing that this should be default, but if you define a bunch of function factories, you end up with a lot of essentially empty lines and reduced available line width for the actual implementations.

@MichaelChirico
Copy link
Collaborator

Agree it can be useful, and warrants an argument, but also that it should not be the default choice.

@IndrajeetPatil
Copy link
Collaborator

Okay, thanks for the explaining. Makes sense.

As for the parameter name, how about curly_braces_required (default: TRUE)?

@MichaelChirico
Copy link
Collaborator

is strict=TRUE too glib?

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 7, 2022

strict sounds okay, while not very specific. curly_braces_required would suggest that the other examples mentioned by @MichaelChirico don't lint, which they always should.

@salim-b salim-b linked a pull request Oct 15, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants