Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add assignment_as_infix = TRUE to indentation_linter() #1812

Merged
merged 12 commits into from
Dec 31, 2022

Conversation

AshesITR
Copy link
Collaborator

@AshesITR AshesITR commented Dec 8, 2022

fixes #1800

Currently, this only suppresses indentation for descendants of <--triggered indents and only if these descendants are

  • infix operators
  • not within a function call

I'm not 100% sure this fixes all cases, but I added a more complex test case to ensure it works as intended.
Let me know if you can think of other test cases that should be included.

Also, let me know if the exception should apply to other assignment-like operators, such as EQ_ASSIGN, EQ_SUB, EQ_FORMALS, ...

One case where we probably still have a false-positive is within lapply() and friends. That seems hard if not impossible to fix:

lapply(x,
  function(e) {
    temp_var <-
      e +
      42
  }
)

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2022

Codecov Report

Merging #1812 (e36addf) into main (e0ebf6b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1812   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         112      112           
  Lines        4841     4852   +11     
=======================================
+ Hits         4786     4797   +11     
  Misses         55       55           
Impacted Files Coverage Δ
R/indentation_linter.R 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 8, 2022

Link rot check is getting HTTP 500s at the moment. All other checks pass ✔️

Copy link
Collaborator

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

thanks!

@AshesITR
Copy link
Collaborator Author

AshesITR commented Dec 8, 2022

Thanks for your feedback.
I added OP-LEFT-BRACE to the resetting tokens and EQ_SUB, EQ_ASSIGN and EQ_FORMALS to the suppressing tokens.
The new tests cover all the new exceptions.

I hope the comment is more clear on how the XPath works now.

@AshesITR
Copy link
Collaborator Author

@MichaelChirico LMK when you have the time to review. I want to avoid spamming merges.

@@ -97,7 +118,8 @@
#' - <https://style.tidyverse.org/functions.html#long-lines-1>
#'
#' @export
indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "always", "never")) {
indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "always", "never"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i usually prefer each argument with defaults on its own line once >1 line is used, WDYT? not sure it's a stated rule, but it's similar to the long lines rule for calls.

@@ -261,6 +297,16 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al
})
}

find_new_indent <- function(current_indent, change_type, indent, hanging_indent) {
if (change_type == "hanging") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe a switch() is more appropriate? change_type feels pretty enum-like

")

# test brace restorator
code_infix_3 <- trim_some("
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's some horrible code 😄

Copy link
Collaborator

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

Only nits left, great work!

@AshesITR AshesITR merged commit f7b91bd into main Dec 31, 2022
@IndrajeetPatil IndrajeetPatil deleted the fix/1800-assign-double-indentation branch April 28, 2023 06:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive in indentation_linter()?
4 participants