Enable detecting missing terminal newline - #774
MichaelChirico wants to merge 5 commits into
Conversation
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Three issues to address before merging. The code-action tests and both new diagnostics tests passed with default lintr settings; the multiline Unicode quick fix also passed.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Re-reviewed 1012da4. The three original findings are addressed: package namespace context is preserved, the fallback consistently uses UTF-8, and the folding/symbol fixture regression is gone. I re-ran the package-context and Latin-1 reproducers with lintr 3.4.0, checked inline and file exclusions, and ran test-lintr, test-code-action, test-folding, and test-symbol with default lintr settings; all passed. I am resolving those three threads.
One remaining issue is noted inline: the fallback is no longer reached for untitled documents. The direct helper test passes, but diagnose_file() does not use it for those documents.
All three R CMD check jobs are green. Coverage shard 3 is still red at test-lintr.R:58-59 because no diagnostic notification was received; I did not reproduce that failure in the focused local run.
| if (!terminal_newline && !lintr_supports_text_terminal_newline()) { | ||
| has_terminal_newline_lint <- any(vapply(lints, function(lint) { | ||
| identical(lint$message, "Add a terminal newline.") | ||
| }, logical(1L))) | ||
| if (!has_terminal_newline_lint) { | ||
| lints <- c(lints, lint_without_terminal_newline(path, content, linters, cache)) |
There was a problem hiding this comment.
[P2] Apply the fallback to untitled documents too
This fallback is now inside the nzchar(path) branch, so an unsaved untitled: document goes through the final else and never receives the missing-newline diagnostic on lintr 3.4.0. I reproduced diagnose_file("untitled:1", "x <- 1", cache = FALSE) returning an empty list at this commit, while the initial PR implementation, the same content with a file URI, and lint_without_terminal_newline("", ...) all report Add a terminal newline.. Please apply the fallback after both plain-R linting branches and add coverage through diagnose_file() for an untitled URI; the current helper-only untitled test cannot catch this gap.
There was a problem hiding this comment.
Thanks! Done
|
Is there some known flakiness for this shard? Wondering if I'm fighting a chimera here or if I should expect some payoff to continued efforts :) |
|
My conclusion for
There is also a lower-priority cache issue: when caching is enabled, each fallback call leaves a persistent cache entry for a fresh temporary filename. Three identical calls produced four cache files. Using On the coverage question: the same initial diagnostics-notification failure already occurs on the PR's base commit (base shard 3 log), so the current red shard is evidence of a pre-existing problem. All three R CMD checks pass. My focused run with lintr 3.4.0 passed 283 assertions across diagnostics, code actions, folding, and symbols; a multiline Unicode quick-fix check also passed. GitHub currently reports merge conflicts. I recommend resolving those and the two P2 issues, then rerunning the relevant checks before merging. |
2 participants
Towards #772; written by Gemini.
I think a lot of this is written defensively on the assumption that {lintr} does not accept its corresponding change: r-lib/lintr#3132
I think this could be simplified pretty handily if we instead wait for that PR to be included on CRAN and then let {languageserver} require a very recent {lintr} version.