search

r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.16k stars 184 forks source link

Normalize output to Forward slash #2613

Closed olivroy closed 2 weeks ago

olivroy commented 2 weeks ago

Minor annoyance, but thought I'd share the PR in case there is interest

it is just to fix a minor inconsistency between Unix and Windows.

x <- lint(path)
names(x)

This basically just changes that and makes sure that the names of the linted object are the same on both platforms with forward slash

MichaelChirico commented 2 weeks ago

Thanks! makes sense to me. admittedly, it feels weird to override the platform settings and force '/', but I don't know of any OS in current use that doesn't understand '/' so it's kinda moot.

I think more importantly, this PR suffers from threat of future regression -- there's nothing stopping us from using file.path()/normalizePath() the "old" way going forward.

Could you file a follow-up FR about a linter for this purpose? we can hash out design details there. It needn't impede this PR from being merged but needs to be recorded before slipping through the cracks.

olivroy commented 2 weeks ago

Thanks for looking into it! I did this quickly to see if it could work (and if there was interest).

I will look into removing changes, and see which ones are actually necessary. If there are <10, this would be less scary, and it could be documented where this is needed and not?

olivroy commented 2 weeks ago

Some tests feel redundant now (they are just testing the equivalency of paths). The only problem is that withr::local_tempfile() seems to create files with \\, so maybe a helper is needed to normalize path to fslash

local_tempfile_f <- function(...) {
  file <- withr::local_tempfile()
  normalize_path(file)
}
MichaelChirico commented 2 weeks ago

The approach of just using normalizePath() as an undesirable function looks good enough to me for now. We can think about a new linter upon user request later.