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

New function_brace_linter #987

Merged
merged 7 commits into from
Mar 27, 2022
Merged

New function_brace_linter #987

merged 7 commits into from
Mar 27, 2022

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

R/function_brace_linter.R Outdated Show resolved Hide resolved
@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