search

r-lib / styler

Non-invasive pretty printing of R code
https://styler.r-lib.org
Other
725 stars 71 forks source link

Dealing with warnings in tests on Windows #1161

Closed IndrajeetPatil closed 10 months ago

IndrajeetPatil commented 10 months ago

cf. https://github.com/r-lib/styler/actions/runs/7078474055/job/19263996467

══ Warnings ════════════════════════════════════════════════════════════════════
── Warning ('test-io.R:11:3'): non-ASCII characters are handled properly for file styling ──
using locale code page other than 65001 ("UTF-8") may cause problems
Backtrace:
    ▆
 1. └─withr::with_locale(...) at test-io.R:11:3
 2.   └─withr::local_locale(new)
 3.     └─withr:::set_locale(cats)
 4.       └─base::mapply(Sys.setlocale, names(cats), cats)
 5.         └─base (local) `<fn>`(dots[[1L]][[1L]], dots[[2L]][[1L]])
── Warning ('test-parsing.R:21:3'): repreated parsing solves wrong parent assignment ──
'R -q -e "styler::cache_deactivate(); styler::style_file(\"C:\Users\RUNNER~1\AppData\Local\Temp\RtmpQnZB6B/working_dir\RtmpoJN3xq\styler161c67386af4/repeated_parsing-in.R\")"' execution failed with error code 1
Backtrace:
    ▆
 1. └─styler:::calls_sys(...) at test-parsing.R:21:3
 2.   └─base::shell(sys_call, ...) at styler/R/utils.R:105:5
lorenzwalthert commented 10 months ago

the wrong parent assignment bug was fixed long time ago and we don’t even support R versions that old with styler, so I guess the test can be deleted. Context; https://github.com/r-lib/styler/issues/187

IndrajeetPatil commented 10 months ago

I am not really sure how to get rid of this warning:

using locale code page other than 65001 ("UTF-8") may cause problems

Using the specified locale code instead leads to a different warning about the OS not honouring the requested locale.

lorenzwalthert commented 10 months ago

A web search yields many (apparently relevant) results, e.g. also https://github.com/Rdatatable/data.table/issues/5696. Maybe these can help?

IndrajeetPatil commented 10 months ago

Maybe these can help?

Only if suppressWarnings() is an option. That's what {data.table} seems to be doing 🙈

But maybe we can do the same thing. I agree with what Michael says in the PR you mentioned:

Probably we should just suppressWarnings() since this is in tests, any "problems" would be detected by a test failure. Alternatively, we might check if the "new" (4.2.0) Sys.setlanguage() will work for those tests.