Skip to content

Conversation

MichaelChirico
Copy link
Collaborator

Part of #962

Shall we mark this one as default? I don't see it called out specifically in the style guide.

@MichaelChirico
Copy link
Collaborator Author

We were not totally sure what to do with a case like we see in lintr:

function(expr) sprintf(
"%s() should take two arguments, with the first starting with 'lib' and the second starting with 'pkg'.",
get_hook(expr)
),

In the end we decided to enforce that having braces. But we might skip linting this type of usage optionally.

@AshesITR
Copy link
Collaborator

AshesITR commented Mar 23, 2022

https://style.tidyverse.org/syntax.html#inline-statements

It's written explicitly for if () but I think it can be generalized to functions. Happy to have this as a default.

Also note the implicit mention of a leading { in https://style.tidyverse.org/functions.html#long-lines-1

cc @jimhester

@AshesITR
Copy link
Collaborator

So, do we make it a default?

@MichaelChirico
Copy link
Collaborator Author

So, do we make it a default?

Yes. Any thoughts on the edge case-y example in the above comment? #987 (comment)

@AshesITR
Copy link
Collaborator

I'd lint it. It's not super easy to see at first glance that there is no closing parenthesis on the function definition line.

AshesITR
AshesITR previously approved these changes Mar 26, 2022
@AshesITR AshesITR merged commit 4270c61 into master Mar 27, 2022
@MichaelChirico MichaelChirico deleted the function_brace branch March 30, 2022 16:25
richfitz added a commit to mrc-ide/outpack-r that referenced this pull request Jul 27, 2022
A new modification of brace_lintr was added in lintr 3.0.0;
unfortunately this is not configurable without entirely disabling
the linter which does other desirable things entirely.

See

r-lib/lintr#987 (introduces new linter)
r-lib/lintr#1092 (combines linters unconfigurably)
richfitz added a commit to mrc-ide/outpack-r that referenced this pull request Jul 27, 2022
A new modification of brace_lintr was added in lintr 3.0.0;
unfortunately this is not configurable without entirely disabling
the linter which does other desirable things entirely.

See

r-lib/lintr#987 (introduces new linter)
r-lib/lintr#1092 (combines linters unconfigurably)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants