Skip to content

Normalize output to Forward slash #2613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 20, 2024
Merged

Normalize output to Forward slash #2613

merged 10 commits into from
Jun 20, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jun 19, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

olivroy commented Jun 19, 2024

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
Copy link
Collaborator Author

olivroy commented Jun 19, 2024

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
Copy link
Collaborator

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.

@IndrajeetPatil IndrajeetPatil merged commit 1c2c8a9 into r-lib:main Jun 20, 2024
18 checks passed
@olivroy olivroy deleted the forward-slash branch August 6, 2024 00:18
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.

3 participants