Skip to content

feat(new linter): added a new linter #830

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

Merged
merged 12 commits into from
Jul 14, 2021
Merged

feat(new linter): added a new linter #830

merged 12 commits into from
Jul 14, 2021

Conversation

kpagacz
Copy link
Contributor

@kpagacz kpagacz commented Jul 7, 2021

Hi all,

I have added a new linter that checks for a space after the closing parenthesis of the list of formal function arguments in cases where no braces are used and the function definition spans only a single line.

Examples:

function()print("test") # BAD

function() print("test") # OK

function()
  print("test")               # OK
  • Added a new linter that checks for a space between the right parenthesis and the function body in function definitions that span one line and don't use braces

Closes #809

* Added a new linter that checks for a space between the right parenthesis and the function body in function definitions that span one line and don't use braces

Closes #809
@kpagacz
Copy link
Contributor Author

kpagacz commented Jul 7, 2021

Locally, some of the checks don't pass for me. I have branched off master, is this intended?
The checks that don't pass:
image

image

@AshesITR
Copy link
Collaborator

AshesITR commented Jul 7, 2021

Hi @kpagacz,

thanks for taking the time to write a PR!

Locally, some of the checks don't pass for me. I have branched off master, is this intended?
The checks that don't pass:
image

image

I had similar issues on my Windows machine.
One or more of the following may help:

  1. Make sure your lintr Project directory is on your system (C:) drive and contains no spaces
  2. Make sure to start the checks from a fresh R session that has no packages attached

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, just a few things need changing.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you.
I think this is ready to merge. Would you add a line to NEWS.md and update the roxygen documentation to reflect the extensions?

@kpagacz
Copy link
Contributor Author

kpagacz commented Jul 10, 2021

Done. Thanks for taking the time to review!

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AshesITR
Copy link
Collaborator

@jimhester @MichaelChirico @renkun-ken @russHyde do we want this in the default linters?
It's mentioned in the tidyverse style guide.

@AshesITR AshesITR self-requested a review July 14, 2021 06:21
@MichaelChirico
Copy link
Collaborator

yes agree. IINM styler::style_file also fixes this

expect_lint("if (TRUE)test", msg, linter)
expect_lint("while (TRUE)test", msg, linter)
expect_lint("for (i in seq_along(1))test", msg, linter)
expect_lint("\\()test", msg, linter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda syntax should only be checked in R >= 4.0.
Curiously, the test also fails for R 4.0.

Can you investigate?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for failure on 4.0 itself, wasn't there a bug with utils::getParseData() on that release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to look into running that particular test only for R>=4.0 and will check if the parsed tree is somehow wrong for version 4.0

Copy link
Contributor Author

@kpagacz kpagacz Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the failure on the R version 4.0.5 (I assume that's the one running there). The new lambda syntax was introduced in the version 4.1.0 as per this NEWS document: https://cran.r-project.org/doc/manuals/r-release/NEWS.pdf (3rd point on the 3rd page). For obvious reasons parsing () lambdas returns an error on the version 4.0.x
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad re the version number. The condition needs to be on R version >= 4.1.0, not 4.0.0
Do you need help with adding the linter to the defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the necessary skip condition.

@russHyde
Copy link
Collaborator

russHyde commented Jul 14, 2021

I don't think unbraced blocks of the form

f <- function(x)
    print(x)

are allowed in the tidyverse style (see section "inline statements" in https://style.tidyverse.org/syntax.html#control-flow); though they are allowed if on a single line.
So this linter doesn't seem to fit with the tidyverse style, and IMO shouldn't be in the defaults

@MichaelChirico
Copy link
Collaborator

Russ, AIUI that example is just to show another usage that wouldn't be flagged by this linter, rather than showing code that is "OK".

That is, the code maybe should be flagged, but it would be by a different linter, unless you are proposing to extend the scope of this linter.

@jimhester
Copy link
Member

I think this could be added to the default linters.

@russHyde
Copy link
Collaborator

OK cool.

@AshesITR AshesITR merged commit e7dcac7 into r-lib:master Jul 14, 2021
@kpagacz kpagacz deleted the paren_body_linter branch July 15, 2021 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linter for space needed after right parenthesis of function symbol formals
6 participants