Skip to content
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

simplify temporary language settings #676

Merged
merged 7 commits into from
Dec 5, 2020
Merged

Conversation

MichaelChirico
Copy link
Collaborator

Originally wrote this because I noticed another place where we relied on English locale:

https://github.com/jimhester/lintr/blob/9104a004d50c66d3d77cc4e0d4a2c83067f8b43a/R/get_source_expressions.R#L89

But that's covered by #663 already.

Anyway, I think this approach looks nicer. Considered an approach like with_lang("en", {...}) but the expressions within {...} would be huge, don't think it would actually be better.

Also note that before, we weren't actually restoring LANGUAGE in the event that it was currently unset, e.g.

ORIG = Sys.getenv("LANGUAGE", unset=NA)

Sys.unsetenv("LANGUAGE")

old = Sys.getenv("LANGUAGE")
# do stuff
Sys.setenv(LANGUAGE = old)

# initially, LANGUAGE was unset, now it's set equal to nothing
Sys.getenv("LANGUAGE", unset=NA)
# [1] ""

# true reset. Sys.setenv(LANGUAGE = NA) actually sets "NA"
if (is.na(ORIG)) Sys.unsetenv("LANGUAGE") else Sys.setenv(LANGUAGE = ORIG)

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 5, 2020

Are you sure the current behaviour isn't resetting env?

"LANGUAGE" %in% names(Sys.getenv())
#> [1] FALSE
Sys.setenv(LANGUAGE = "en")
"LANGUAGE" %in% names(Sys.getenv())
#> [1] TRUE
Sys.setenv(LANGUAGE = "")
"LANGUAGE" %in% names(Sys.getenv())
#> [1] FALSE

Tested on R 4.0.3 @ Windows 10.

Also, if we do make a helper, we should use it in all places (I remember R/expect_lint.R and tests/testthat/test-error.R)

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Dec 5, 2020

This might be a platform-dependent thing (I'm on Linux Mint ~ Ubuntu):

Sys.unsetenv("LANGUAGE")
"LANGUAGE" %in% names(Sys.getenv())
# [1] FALSE
Sys.setenv(LANGUAGE="")
"LANGUAGE" %in% names(Sys.getenv())
# [1] TRUE

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 5, 2020

Okay then we should fix it in all places. Thanks for pointing it out!

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 5, 2020

#670 causes spurious lints here. I'll whip up a quick fix PR.

@MichaelChirico
Copy link
Collaborator Author

For tests/testthat/test-error.R we'd have to either extract with ::: or re-define it, is it worth it?

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 5, 2020

Using internal functions in tests seems okay to me.

@MichaelChirico
Copy link
Collaborator Author

OK done

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelChirico MichaelChirico merged commit 4d93b8d into master Dec 5, 2020
@MichaelChirico MichaelChirico deleted the english-message branch December 5, 2020 22:51
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.

2 participants