Add param `function_braces` to `brace_linter()` #2240

salim-b · 2023-10-15T19:40:15Z

codecov-commenter · 2023-10-15T19:43:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (106dfb4) to head (70f1a4f).

❗ Current head 70f1a4f differs from pull request most recent head bd7afa7. Consider uploading reports for the commit bd7afa7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2240   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         125      125           
  Lines        5715     5729   +14     
=======================================
+ Hits         5611     5625   +14     
  Misses        104      104

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AshesITR · 2023-10-16T05:17:54Z

This seems too aggressive at removing lints.
The following are meant to lint even with the option enabled:

function(x)
  AWrapper(y, x)

function(x)
  AWrapper(
    y,
    x
  )

See #1807 (comment)

So omitting braces is only okay if the next (and only) token that makes up the function body starts on the same line, e.g.

function(x) AWrapper(
  y,
  x
)

salim-b · 2023-10-16T11:11:57Z

So omitting braces is only okay if the next (and only) token that makes up the function body starts on the same line

I see, sorry didn't look at the mentioned issue closely enough then.

Personally, I don't care too much about these special variations, so I'd be happy with having the require_multi_line param as it is now. And I'm not familiar enough with XPath to implement it the way you outline, I'm afraid. So I hope it's okay I let this up to you guys 🙂

MichaelChirico · 2023-10-16T15:41:15Z

what's supported by this PR is not style guide-compliant, but that's ok, as long as the default is compliant. Agree that this does not close the referenced issue (though related -- it's even more lax). maybe we can have this parameter take 3 values instead:

function_braces = c("always", "not_inline", "never")

(the naming of the middle option is as usual tricky)

if you're interested, give it a try to implement the middle option -- I suspect it's doable with some effort for you.

Otherwise, feel free to implement the design above with a stop() for "not yet implemented" on the middle option & we'll handle it in follow up (before the subsequent release).

salim-b · 2023-10-18T00:35:14Z

@MichaelChirico Thanks for the encouragement, I think I got it. IMHO "multi_line" is a better name for the middle option than "not_inline" (although I'm no native speaker; but "inline function" seems to evoke different concepts).

Open question: When using function_braces = c("always", "multi_line", "never") in the brace_linter() definition (with function_braces <- match.arg(function_braces) afterwards), the default changes. I wouldn't care personally, but the current tests do, especially

lintr/tests/testthat/test-expect_lint.R

Line 63 in 4a8b931

expect_lint_free(test_path("dummy_packages", "clean_subdir", "r"))

Which option do you prefer?

Change the default (and adapt the dummy package content accordingly).
Set function_braces = "multi_line" in the definition of brace_linter() .
Set function_braces = c("multi_line", "always", "never") in the definition of brace_linter(). This one results in a less intuitive function signature.

AshesITR · 2023-10-18T05:20:34Z

The default behavior should not change, because it aligns to the style guide.

Also, IINM, allow_single_line becomes deprecated by this PR and should be replaced by function_braces = "multi_line" with a warning.

Also we have four levels I think:

# lint on "always"
function(x) x

# lint on "multi_line" and "always"
function(x) wrapper(
  x,
  y
)

# lint on "???", "multi_line" and "always"
function(x)
  wrapper(
    x,
    y
  )

"never" would disable this kind of lint.

salim-b · 2023-10-18T16:03:02Z

The default behavior should not change, because it aligns to the style guide.

Ok. Still bears the question which brace_linter() signature you prefer, i.e. option 2 or 3 above.

Also, IINM, allow_single_line becomes deprecated by this PR and should be replaced by function_braces = "multi_line" with a warning.

I don't think so. function(x) { x + 4 } will be linted by brace_linter(allow_single_line = FALSE, function_braces = "multi_line"), but not by brace_linter(allow_single_line = TRUE, function_braces = "multi_line"). This is the way it should be, no?

Also we have four levels I think

I think we can go with three options only, so brace_linter(function_braces = "multi_line") will lint both of the following:

function(x)
  x + 4
  
function(x) x +
  4

AshesITR · 2023-10-18T19:36:07Z

The default behavior should not change, because it aligns to the style guide.

Ok. Still bears the question which brace_linter() signature you prefer, i.e. option 2 or 3 above.

Option 3 seems to be viable.

Also, IINM, allow_single_line becomes deprecated by this PR and should be replaced by function_braces = "multi_line" with a warning.

I don't think so. function(x) { x + 4 } will be linted by brace_linter(allow_single_line = FALSE, function_braces = "multi_line"), but not by brace_linter(allow_single_line = TRUE, function_braces = "multi_line"). This is the way it should be, no?

🤦 sorry, I was confused.
You are right.

Also we have four levels I think

I think we can go with three options only, so brace_linter(function_braces = "multi_line") will lint both of the following:
function(x)
  x + 4
  
function(x) x +
  4

I think the former should lint, but the latter isn't easily distinguishable in the AST from

function() wrapper(
  x,
  y
)

so it shouldn't lint (at least for some value of function_braces).

salim-b · 2023-10-18T19:39:54Z

so it shouldn't lint (at least for some value of function_braces).

It won't lint for brace_linter(function_braces = "never"). Is that ok for you?

AshesITR · 2023-10-18T19:42:21Z

I would like to have an option that distinguishes between function definitions where the body starts on the same line, like

function(x) x +
  4

, and those where the body does not start on the same line, like

function(x)
  x + 4

salim-b · 2023-10-18T20:38:17Z

I would like to have an option that distinguishes between function definitions where the body starts on the same line, like (...), and those where the body does not start on the same line, like (...)

Ok. I'm afraid I can't help with that. But you could extend the function_braces param with a forth option not_start_on_same_line¹ in the future. I've updated this PR with the three-option solution.

Hopefully with a better name 😄 ↩

AshesITR · 2023-10-18T20:50:14Z

Thanks for the effort, I'll try and take a look some time this week to see if I can implement the fourth option.
Name suggestions welcome 😆

AshesITR · 2023-10-19T05:55:21Z

@salim-b PTAL, I named it "not_inline" for now.
Please refrain from force-pushing since I've cloned your branch now and it would cause divergent histories.
We squash all commits when a PR is merged into main, so don't worry about creating too many commits.

AshesITR · 2023-10-19T06:00:08Z

Also, brace_linter(function_braces = "always") effectively prohibits inline function definitions. Not sure whether we should

ignore this fact
warn the user about using function_braces = "always" without allow_single_line = TRUE
implicitly set allow_single_line = TRUE if function_braces = "always"
carve out allow_single_line = FALSE to allow function(x) { x } if and only if function_braces = "always".

What do you think, @MichaelChirico?

AshesITR · 2023-10-19T06:06:23Z

We should also discuss what to do about multi-line function headers.

# these all trigger "always", "multi_line" and "not_inline" atm
function(
    a,
    b
) a + b

function(
    a,
    b
) a + 
  b

function(
    a,
    b
)
  a + b

# this forces allow_single_line = TRUE to be set
function(
    a,
    b
) { a + b }

# this never lints
function(
    a,
    b
) {
  a + b
}

I'm fine with this behavior, but it does make the description of "not_inline" slightly off.
Also, currently there are no tests for these - we should add them.

salim-b · 2023-10-19T16:22:08Z

Also, brace_linter(function_braces = "always") effectively prohibits inline function definitions. Not sure whether we should
1. ignore this fact

2. warn the user about using `function_braces = "always"` without `allow_single_line = TRUE`

3. implicitly set `allow_single_line = TRUE` if `function_braces = "always"`
What do you think, @MichaelChirico?

I'm not sure whether I understand correctly... but didn't you mean to write brace_linter(function_braces = "never") (instead of "always") effectively prohibits inline function definitions?

Anyways, I think option 3 would be bad since allow_single_line is not limited to function definitions.

AshesITR · 2023-10-19T17:21:49Z

I'm not sure whether I understand correctly... but didn't you mean to write brace_linter(function_braces = "never") (instead of "always") effectively prohibits inline function definitions?

I meant "always", see these tests:

https://github.com/r-lib/lintr/pull/2240/files#diff-42f1e9787771f9204786c9d84ece7bc678f89e7cbd6b3d826f9a2e62ba90da5eR325
https://github.com/r-lib/lintr/pull/2240/files#diff-42f1e9787771f9204786c9d84ece7bc678f89e7cbd6b3d826f9a2e62ba90da5eR336

None of these two ways to define an inline function, with or without braces, is acceptable for brace_linter(allow_single_line = FALSE, function_braces = "always").

function(x) x
function(x) { x }

Anyways, I think option 3 would be bad since allow_single_line is not limited to function definitions.

Good point.
Added Option 4.: Carve out allow_single_line = FALSE to allow function(x) { x } if and only if function_braces = "always".

MichaelChirico · 2023-10-19T18:25:31Z

FYI: I'm holding off on review until after release. thanks for your patience.

salim-b · 2023-10-19T22:25:47Z

Added Option 4.: Carve out allow_single_line = FALSE to allow function(x) { x } if and only if function_braces = "always".

I think I'd still prefer option 1. But you and @MichaelChirico are certainly more qualified to decide on that^^

We should also discuss what to do about multi-line function headers. (...) Also, currently there are no tests for these - we should add them.

Since function headers are unrelated to braces, this wouldn't concern brace_linter(), no?

AshesITR · 2023-10-20T05:01:00Z

Yes and we pin down the behavior to make sure it works as intended.

R/brace_linter.R

Co-authored-by: Michael Chirico <[email protected]>

R/brace_linter.R

MichaelChirico · 2024-03-13T00:04:54Z

R/brace_linter.R

+#'   `"always"` requires braces for all function definitions, including inline functions.
+#'   `"not_inline"` requires braces when a function body does not start on the same line as its header.
+#'   `"multi_line"` requires braces when a function definition spans multiple lines.
+#'   `"never"` never requires braces in function bodies.


we don't have to include it in this PR, but there is one more style we might cater to in the future, namely, use braces if and only if they're syntactically required.

Consider

A <- function() { NULL } B <- function() { NULL; NULL } C <- function() NULL D <- function() { NULL } E <- function() NULL F <- function() { NULL NULL } G <- function() invisible( ) H <- function() { invisible( ) }

And the corresponding lint table ( ✔️ ➡️ throw a lint):

function_braces= A B C D E F G H

always ✔️ ✔️ ✔️

not_inline ✔️

multi_line ✔️ ✔️

never

strict ✔️ ✔️ ✔️

(PS we might consider adding any of the above as test cases if they're not yet represented)

Mentioning this here to possibly align on the naming for such a style. If we can't come up with a name that's natural for the current function_braces (or function_bodies), we might need to think of a different parameter name to accommodate. Hope that makes sense.

use braces if and only if they're syntactically required.

What exactly do you mean by "syntactically required"? I'm confused by your examples above, A, D and H are fine without braces (same result)...

What exactly do you mean by "syntactically required"?

i.e., "without braces, the code either wouldn't parse, or would parse incorrectly". In other words "function uses braces" if and only if "function has more than one expression" (I think that should be equivalent to length(body(f)) > 1)

A, D and H are fine without braces (same result)...

Precisely why they would throw a lint (under "strict" mode, or maybe "minimalist" mode is more evocative) -- that style ditches { whenever possible. In A/D/H, ditching { doesn't impact the validity of the code --> lint

R/brace_linter.R

…fn-brace # Conflicts: # R/brace_linter.R

This reverts commit 5eaae61 (which breaks tests).

Co-authored-by: Michael Chirico <[email protected]>

…fn-brace # Conflicts: # R/brace_linter.R

salim-b

5eaae61 broke tests, thus I reverted it.

salim-b · 2024-06-06T16:45:59Z

@MichaelChirico This PR has been sitting here for ~~almost~~ more than three months without any further changes. I'd love to get it merged if possible.

katrinabrock · 2024-08-05T12:13:42Z

thanks @salim-b for putting this together. I'm running off your branch for now. look forward to seeing this merged.

NEWS.md

MichaelChirico · 2024-08-06T17:37:39Z

R/brace_linter.R

 #'
-#' @param allow_single_line if `TRUE`, allow an open and closed curly pair on the same line.
+#' @param allow_single_line If `TRUE`, allow an open and closed curly pair on the same line.
+#' @param function_bodies Whether to require function bodies to be wrapped in curly braces. One of


"Whether" kind of implies a binary choice

Suggested change

#' @param function_bodies Whether to require function bodies to be wrapped in curly braces. One of

#' @param function_bodies Character string governing when function bodies without curly braces should generate lints. One of

Also, as @AshesITR points out, I think we should be careful here to specify "function body" for all the rules, since a function definition includes the parameters and those can take up many lines even if the function itself doesn't (not that we expect to see that except in strange cases).

R/brace_linter.R

MichaelChirico · 2024-08-06T17:50:14Z

tests/testthat/test-brace_linter.R

Suggested new test case:

foo <- function(x) ({ x + 1 })

(I think it should lint, this is an unusual way to declare a function that should be discouraged in favor of plain {.)

Also, something like:

foo <- function(x) { x + 4 }

Is slightly different from the current set of cases & worth including as a regression test I think.

salim-b force-pushed the allow-fn-brace branch from 67b1c38 to 63c198b Compare October 18, 2023 20:37

salim-b changed the title ~~Add param require_multi_line to brace_linter()~~ Add param function_braces to brace_linter() Oct 18, 2023

Add param function_braces to brace_linter()

f37b35b

salim-b force-pushed the allow-fn-brace branch from 63c198b to f37b35b Compare October 18, 2023 20:44

refactor, add option not_inline

acf4370

update NEWS bullet

de6989f

Fix typos

1494755

salim-b and others added 3 commits November 10, 2023 06:06

Roxygenize

63ac52f

Merge branch 'main' into allow-fn-brace

25f1ed8

Move NEWS item, clarify it

54c1ed3

MichaelChirico reviewed Mar 12, 2024

View reviewed changes

R/brace_linter.R Outdated Show resolved Hide resolved

salim-b and others added 3 commits March 13, 2024 00:52

Update R/brace_linter.R

555b192

Co-authored-by: Michael Chirico <[email protected]>

Update R/brace_linter.R

a4b54da

Co-authored-by: Michael Chirico <[email protected]>

Use list in roxygen doc

229993a

MichaelChirico reviewed Mar 13, 2024

View reviewed changes

R/brace_linter.R Outdated Show resolved Hide resolved

MichaelChirico reviewed Mar 13, 2024

View reviewed changes

Use 'true' in XPath for clarity

5eaae61

MichaelChirico reviewed Mar 13, 2024

View reviewed changes

R/brace_linter.R Outdated Show resolved Hide resolved

salim-b and others added 8 commits March 13, 2024 01:13

rename param function_braces to function_bodies

34af80d

Merge branch 'allow-fn-brace' of github.com:salim-b/lintr into allow-…

89cd471

…fn-brace # Conflicts: # R/brace_linter.R

Revert "Use 'true' in XPath for clarity"

ecbe9b6

This reverts commit 5eaae61 (which breaks tests).

Fix last merge

54d257f

Update R/brace_linter.R

3e13651

Co-authored-by: Michael Chirico <[email protected]>

Roxygenize

9e27450

Merge branch 'allow-fn-brace' of github.com:salim-b/lintr into allow-…

13bd010

…fn-brace # Conflicts: # R/brace_linter.R

Update tests

bd7afa7

salim-b commented Mar 13, 2024

View reviewed changes

Merge branch 'main' into allow-fn-brace

9e1568c

Merge branch 'main' into allow-fn-brace

9fe4c56

MichaelChirico reviewed Aug 6, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

grammar

f4a3a70

MichaelChirico reviewed Aug 6, 2024

View reviewed changes

R/brace_linter.R Outdated Show resolved Hide resolved

use 'true' over '1' for boolean literal in XPath

4223eea

MichaelChirico reviewed Aug 6, 2024

View reviewed changes

R/brace_linter.R Outdated Show resolved Hide resolved

slightly improve readability of XPath

7620e37

MichaelChirico reviewed Aug 6, 2024

View reviewed changes

Merge branch 'main' into allow-fn-brace

2dc893e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add param `function_braces` to `brace_linter()` #2240

Add param `function_braces` to `brace_linter()` #2240

salim-b commented Oct 15, 2023 •

edited

Loading

codecov-commenter commented Oct 15, 2023 •

edited

Loading

AshesITR commented Oct 16, 2023

salim-b commented Oct 16, 2023

MichaelChirico commented Oct 16, 2023 •

edited

Loading

salim-b commented Oct 18, 2023 •

edited

Loading

AshesITR commented Oct 18, 2023 •

edited

Loading

salim-b commented Oct 18, 2023

AshesITR commented Oct 18, 2023

salim-b commented Oct 18, 2023

AshesITR commented Oct 18, 2023

salim-b commented Oct 18, 2023

AshesITR commented Oct 18, 2023

AshesITR commented Oct 19, 2023

AshesITR commented Oct 19, 2023 •

edited

Loading

AshesITR commented Oct 19, 2023 •

edited

Loading

salim-b commented Oct 19, 2023

AshesITR commented Oct 19, 2023 •

edited

Loading

MichaelChirico commented Oct 19, 2023

salim-b commented Oct 19, 2023 •

edited

Loading

AshesITR commented Oct 20, 2023

MichaelChirico Mar 13, 2024

MichaelChirico Mar 13, 2024

salim-b Mar 13, 2024

MichaelChirico Aug 6, 2024

salim-b left a comment •

edited

Loading

salim-b commented Jun 6, 2024 •

edited

Loading

katrinabrock commented Aug 5, 2024

MichaelChirico Aug 6, 2024

MichaelChirico Aug 6, 2024

MichaelChirico Aug 6, 2024

MichaelChirico Aug 6, 2024

`function_braces=`	A	C	D	E	G	H
always		✔️		✔️	✔️
not_inline				✔️
multi_line				✔️	✔️
never
strict	✔️		✔️			✔️

	#' @param function_bodies Whether to require function bodies to be wrapped in curly braces. One of
	#' @param function_bodies Character string governing when function bodies without curly braces should generate lints. One of

Add param function_braces to brace_linter() #2240

Are you sure you want to change the base?

Add param function_braces to brace_linter() #2240

Conversation

salim-b commented Oct 15, 2023 • edited Loading

codecov-commenter commented Oct 15, 2023 • edited Loading

Codecov Report

AshesITR commented Oct 16, 2023

salim-b commented Oct 16, 2023

MichaelChirico commented Oct 16, 2023 • edited Loading

salim-b commented Oct 18, 2023 • edited Loading

AshesITR commented Oct 18, 2023 • edited Loading

salim-b commented Oct 18, 2023

AshesITR commented Oct 18, 2023

salim-b commented Oct 18, 2023

AshesITR commented Oct 18, 2023

salim-b commented Oct 18, 2023

Footnotes

AshesITR commented Oct 18, 2023

AshesITR commented Oct 19, 2023

AshesITR commented Oct 19, 2023 • edited Loading

AshesITR commented Oct 19, 2023 • edited Loading

salim-b commented Oct 19, 2023

AshesITR commented Oct 19, 2023 • edited Loading

MichaelChirico commented Oct 19, 2023

salim-b commented Oct 19, 2023 • edited Loading

AshesITR commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salim-b left a comment • edited Loading

Choose a reason for hiding this comment

salim-b commented Jun 6, 2024 • edited Loading

katrinabrock commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add param `function_braces` to `brace_linter()` #2240

Add param `function_braces` to `brace_linter()` #2240

salim-b commented Oct 15, 2023 •

edited

Loading

codecov-commenter commented Oct 15, 2023 •

edited

Loading

MichaelChirico commented Oct 16, 2023 •

edited

Loading

salim-b commented Oct 18, 2023 •

edited

Loading

AshesITR commented Oct 18, 2023 •

edited

Loading

AshesITR commented Oct 19, 2023 •

edited

Loading

AshesITR commented Oct 19, 2023 •

edited

Loading

AshesITR commented Oct 19, 2023 •

edited

Loading

salim-b commented Oct 19, 2023 •

edited

Loading

salim-b left a comment •

edited

Loading

salim-b commented Jun 6, 2024 •

edited

Loading