rhino icon indicating copy to clipboard operation
rhino copied to clipboard

Topic: Linting and formatting R

Open kamilzyla opened this issue 2 years ago • 2 comments

We have faced situations where changes introduced by styler are flagged by lintr, or are simply undesirable. Due to its flaws, we typically don't use styler in regular development workflow, but perhaps as a one-off at the beginning of a project, when inheriting a lot of ill-formatted legacy code.

Ideas

  1. Linter: Check for ... used in box::use() statements (box::use(shiny[...])).
  2. Styler: Provide a default paths = c("app", "tests") argument. This could encourage users to run format_r() more often and give us more feedback - in what situations it's too aggressive / inconsistent with lint_r(). See #109 for discussion.
  3. Styler: In addition to (2) - add a prompt to confirm the styling.

kamilzyla avatar Sep 21 '21 06:09 kamilzyla

@kamilzyla maintainer of {styler} here. Consistency with {lintr} is certainly a goal we are working towards and we are open to concrete reporting. Also, there are different levels of invasiveness for styler, e.g. you can only format token, indention, linebreaks or spaces, see https://styler.r-lib.org/articles/styler.html#scope-what-to-style.

lorenzwalthert avatar Apr 09 '22 23:04 lorenzwalthert

@kamilzyla I'd like to add ideas for a linter rules based on my previous experiences:

  • Linter: flagging reactive declarations inside the observers as warning/error. Declaring reactive's in observers doesn't yield an immediate error but it can cause a serious problems (chain reactions) further down the line (unnecessary performance drops, strange errors with values etc.). It's a bad practice to do it and it would be nice if system automatically detected it
  • Linter: checking if functions declared in the box::use() are actually used in the file. If we use box properly and only explicitly declare functions we need, there's still a risk that we will keep something unused in the box even if we get rid of it in the code. This may later "invisibly" increase number of dependencies. I imagine it may be more tricky to do with loading scripts instead of packages though
  • (this may be very complicated to implement) Linter: if we dabble in JS in shiny there's a chance we communicate JS with shiny through Shiny.setInputValue('app-moduleName-inputName', value);. This works nicely, but it also means we create inputs in two places: R scripts and JS scripts which makes managing them harder. A rule that would detect the inputs declared in JS and check if those are actually used anywhere in R would be nice to ease the pain of managing them

Leszek-Sieminski avatar Oct 05 '22 09:10 Leszek-Sieminski