Skip to content

Fix #3089 - symlinked tempfiles should still report issues#3726

Merged
hsanson merged 1 commit intodense-analysis:masterfrom
joshpencheon:fix-linters-with-symlinked-reports
Apr 2, 2025
Merged

Fix #3089 - symlinked tempfiles should still report issues#3726
hsanson merged 1 commit intodense-analysis:masterfrom
joshpencheon:fix-linters-with-symlinked-reports

Conversation

@joshpencheon
Copy link
Contributor

Summary

This PR fixes #3089 (+ r-lib/lintr/issues/480), by ensuring that linters reporting issues against resolved symlink tempfile paths are still considered as reporting against tempfile paths.

Detail

For example, on macOS calls to ale#util#Tempname() produce paths of the form /var/folders/..., which are actually symlinks to /private/var/...:

$ pwd -P /var/folders/0r/65637b055vx11s2zwr4f_4k80000gn/T/nvimtH8m0W/15
/private/var/folders/0r/65637b055vx11s2zwr4f_4k80000gn/T

It is possible that a linter reports the physical location of ALE's tempfile of the buffer contents (output from :ALEInfo):

  Command History:
(executable check - success) Rscript
(finished - exit code 0) ['/usr/local/bin/bash', '-c', 'cd ''/Users/josh/code/r_demo'' && Rscript --vanil
la -e ''suppressPackageStartupMessages(library(lintr));lint(cache = FALSE, commandArgs(TRUE), with_defaul
ts())'' ''/var/folders/0r/65637b055vx11s2zwr4f_4k80000gn/T/nvimtH8m0W/2/demo.R''']

<<<OUTPUT STARTS>>>
/private/var/folders/0r/65637b055vx11s2zwr4f_4k80000gn/T/nvimtH8m0W/2/demo.R:1:3: style: Use <-, not =, f
or assignment.
a = c("hello", "world")
  ^
<<<OUTPUT ENDS>>>

...which without this PR would not be detected as being a tempfile by ale#path#IsTempName(filename) – preventing the issues being reported.

Some linters will expand the paths of symlinked tempfiles when
reporting them back to ALE. This ensures that if they do, the filename
is still flagged appropriately as being temporary.
@stale
Copy link

stale bot commented Jul 11, 2021

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Jul 11, 2021
@stale stale bot closed this Jul 18, 2021
@yahiaelgamal
Copy link

yahiaelgamal commented Dec 11, 2022

why did this pull request not get merged? It does solve the problem on my setup (macOS as well).

@rymdbar
Copy link
Contributor

rymdbar commented Apr 1, 2025

Thanks @joshpencheon for your pull-request!

From the looks of it, this probably got closed merely due to unfortunate timing. I'm guessing it got filed when the maintainers were likely lacking the bandwidth to keep up with ale contributions.

To my eyes the patch seemingly looks good, and it still rebases cleanly on current master (067e74f).

Thus I'm reopening this and in the hope that either @hsanson or @w0rp will be able to take notice of it this time.

@rymdbar rymdbar reopened this Apr 1, 2025
@stale stale bot removed stale PRs/Issues no longer valid labels Apr 1, 2025
Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@hsanson hsanson merged commit 2883260 into dense-analysis:master Apr 2, 2025
8 checks passed
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.

ALE not linting .r files with lintr (but working with other filetypes)

4 participants