Repository navigation
build: adopt Arity and enforce R formatting in CI - #1946
Conversation
Add arity-cli as a pinned dev dependency with format:r, format:r:check and lint:r scripts. Layout is now owned by the formatter, so drop the line length and indentation linters from the lintr configuration.
Mechanical change from `pnpm run format:r`. The parsed code is unchanged apart from braces around single-expression bodies.
Apply the safe autofixes (repeat, outer-negation, comparison-negation, fixed-regex).
Add format and lint-arity jobs next to the lintr job, which is renamed to lint-lintr, and document the new scripts for contributors.
9af58b5 to
3c54a6e
Compare
|
@etiennebacher @jolars The difference between Jarl and Arity, as described in the PR body, might be interesting. |
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed at 3c54a6e6f732104ca8b56619771c9be342c6fdff. No blocking findings.
I independently compared the formatting commit across all 58 R files: the parsed expressions match after normalizing single-expression braces. I also inspected the separate autofix commit and the dependency, configuration, and workflow changes. The checked-in tree passes both an uncached formatting check and arity format --verify, so the non-idempotence noted in the evaluation does not prevent this head from satisfying CI.
Local validation on macOS:
- Frozen-lockfile installation of Arity 0.29.0; R format, verification, and Arity lint checks.
- Retained lintr checks for both
R/andsess/. - Extension build and TypeScript test compilation.
- Source identity/bootstrap, manual attach, tool library/consent, jgd preparation, and rstudioapi preparation tests.
- The PR's bundled sess installed into a temporary library: all 1,341 package test results passed.
All 13 hosted checks are also successful, including Node and extension tests on Linux, macOS, and Windows.
|
Thanks for the feedback! I'll take a look at the idempotency issue and other issues. |
Expose the braced fallback through `Ir::FlatChoice` so a trailing function can hug the call prefix on the first formatting pass. Keep leading arguments flat during hug measurement and cover conditional bodies across line widths, indentation sizes, lambdas, and comments. Refs REditorSupport/vscode-R#1946
I think I am actually going to change the current behavior, even without a knob, to always brace (and expand), because this has been chafing at me quite a bit recently. Is this what you want, or are you looking to still keep short functions on one line, but with braces? |
|
Thanks, I think the change is fine. |
Summary
TypeScript in this repository is formatted and linted with oxfmt and oxlint, but R code had no formatter, and the only linter was lintr, which takes about 14 seconds locally and needs an R installation with lintr.
This PR adds Arity (formatter and linter for R, shipped as a prebuilt binary through npm) as a dev dependency, so
pnpm installis all a contributor needs:pnpm run format:r/pnpm run format:r:check: format R files (about 0.2 s for the whole repository)pnpm run lint:r: lint R files (about 0.3 s)Changes
arity-clipinned to 0.29.0 (formatter output depends on the version, same as oxfmt), plusarity.toml.R/used four spaces andsess/used two; the four-space setting only existed becauseindentation_linter(4)was added to keep lintr 3.1 quiet about the existing code (Fix indentation_linter message #1433).line_length_linterandindentation_linterare turned off in both.lintrfiles. Layout is now owned by the formatter. The formatter and those two linters disagree in a few places (multi-lineifconditions, long string literals); see the report below.arity lint --fix(while (TRUE)→repeat,any(!x)→!all(x),!a == b→a != b,fixed = TRUEfor literal patterns).formatandlint-arityjobs inr.ymlrun next to the lintr job, which is renamed fromlinttolint-lintrand otherwise unchanged.CONTRIBUTING.mddescribes the new scripts.Notes for reviewers
parse(keep.source = FALSE)of every file before and after: the parsed code is identical except for braces around single-expression bodies (Arity writesfunction(x) {\n x + 1\n}asfunction(x) x + 1when it fits on one line).sess/DESCRIPTIONis excluded from formatting (description = false) because Arity reorders its fields and build scripts rewrite that file.undefined-symbol,unused-binding: tinytest files andsource()d helpers cannot be resolved statically (lintr'sobject_usage_linteris off for the same reason).coalesce: suggests%||%, which needs R 4.4.internal-function::::is deliberate here.roxygen-param,roxygen-return,shadowed-builtin,string-boundary,unnecessary-nesting: 23 existing findings that are left for a follow-up..git-blame-ignore-revsfile.Why Arity and not Air + Jarl
The only reason to prefer Arity is that it installs through npm, which fits how this repository already manages its tooling. Air (formatter) and Jarl (linter) are more widely used, so I compared them on this code base to check that Arity is not a clearly worse choice. It is not: the formatter is close to Air, and the linter has different strengths from Jarl.
Evaluation report: Arity 0.29.0 vs Air 0.13.0 + Jarl 0.7.1 (and feedback for both projects)
Setup
R/,sess/, andscripts/at 80b16f6arity-cli), Air 0.13.0, Jarl 0.7.1, lintr 3.4.0, R 4.5.2R/: width 120, indent 4;sess/: width 100, indent 2) so that the existing lintr configuration could act as a referee.Formatter: Arity vs Air
+/-lines).lintr)indentation_linteron multi-line conditions insideif (, where lintr wants a hanging indent, and 2 fromline_length_linteron string literals that cannot be split.%in%in"x" %in% y && ...chains, and Air always adding braces toif (cond) return(x).Style differences from Air (about 1,100 lines when going from Air's output to Arity's):
roxygen = falsedisables it) andDESCRIPTION(description = false).Linter: Arity vs Jarl
.lintrwith a lint counterpartequals-na,seq,true-false-symbol,vector-logic)assignment,quotes,pipe_consistency)undefined-symbol: 759undesirable_operator: 320undefined-symbolfindings are tinytest'sexpect_*functions insess/inst/tinytest/; the rest are helpers loaded withsource().unused-binding(27) flags functions that are used from other files.undesirable_operatoris 283:::and 37<<-. Most:::are thesesstests callingsessinternals; Arity recognises the package fromDESCRIPTIONand only reports:::into other packages (6).namespace_hasinsess/R/rstudioapi.R).Feedback for Arity
Non-idempotent formatting. The first pass expands the call, the second pass collapses it again;
arity format --verifyalso reports it. Minimal input:First pass:
Second pass:
Config discovery follows the working directory, not the file. The README says
arity.tomlis "discovered by walking up from each file's directory", butarity format Rrun from the repository root does not readR/arity.toml. Only a config in the working directory or--configis used.No per-file or per-directory rule selection.
ignoreis global andexcludeskips whole files, soundefined-symbolcannot be turned off for test directories only. A way to declare names attached by a test framework (tinytest, testthat) would remove most of our findings.seqmissesseq(length(x)), which lintr and Jarl report.Layout depends on the input in one case.
x <- if (a) b else cstays on one line if it was written on one line and stays expanded if it was written expanded, so the output is not fully determined by the syntax tree.--output jsonprints nothing when there are no findings, rather than[].An option to keep braces on single-expression function bodies would make adoption easier for projects coming from Air.
Feedback for Jarl
A safe fix for
class_equalschanges behaviour. Insess/R/handlers.R:The original deliberately accepts only objects whose class is exactly
"data.frame". For an object withclass(object) <- c("custom", "data.frame"), the original returnsFALSEat this guard and the fixed code continues past it.identical(class(x), "y")is an exact-class test and not equivalent toinherits(), so this fix should not be marked safe.implicit_assignmentreports assignments insideon.exit()(7 findings, such ason.exit(state$con <- old, add = TRUE)). Moving the assignment out of the call, as suggested, would change when it runs.on.exitcould be skipped by default.undesirable_operatorreports:::into the package's own namespace in its own tests (277 of 283 here). Skippingpkg:::whenpkgis the package being linted would remove most of the default noise for packages.pipe_consistencyis silent without a minimum R version. When the rule is selected explicitly but no R version is known, it reports nothing and gives no hint why;--min-r-version 4.2.0makes it report.Limits of this comparison