[SPARK-31571][R] Overhaul stop/message/warning calls to be more canonical #28365

MichaelChirico · 2020-04-27T08:14:38Z

What changes were proposed in this pull request?

Internal usages like {stop,warning,message}({paste,paste0,sprintf} and {stop,warning,message}(some_literal_string_as_variable have been removed and replaced as appropriate.

Why are the changes needed?

CRAN policy recommends against using such constructions to build error messages, in particular because it makes the process of creating portable error messages for the package more onerous.

Does this PR introduce any user-facing change?

There may be some small grammatical changes visible in error messaging.

How was this patch tested?

Not done

…anonical

MichaelChirico · 2020-04-27T08:15:12Z

R/pkg/R/DataFrame.R

@@ -431,7 +431,7 @@ setMethod("coltypes",
                if (is.null(type)) {
                  specialtype <- specialtypeshandle(x)
                  if (is.null(specialtype)) {
-                    stop(paste("Unsupported data type: ", x))
+                    stop("Unsupported data type: ", x)


here, paste was creating an extra space, now fixed

R/pkg/R/client.R

R/pkg/R/utils.R

R/pkg/R/sparkR.R

R/pkg/R/install.R

R/pkg/R/SQLContext.R

R/pkg/R/DataFrame.R

HyukjinKwon · 2020-04-27T09:34:10Z

add to whitelist

HyukjinKwon · 2020-04-27T09:34:15Z

ok to test

SparkQA · 2020-04-27T10:22:18Z

Test build #121897 has finished for PR 28365 at commit 1940eb8.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-27T11:29:53Z

Test build #121900 has finished for PR 28365 at commit e4b8ca9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-04-28T02:09:25Z

cc @felixcheung and @shivaram FYI

SparkQA · 2020-04-28T03:18:48Z

Test build #121941 has finished for PR 28365 at commit b0cf844.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MichaelChirico · 2020-04-28T05:18:57Z

Not sure why the Documents task failed, looks like the roxygen2 part succeeded

srowen · 2020-04-28T18:10:04Z

Sure, but, what about the 1000s in the JVM code this calls too? a little translation isn't going to help anyway. Think about maintaining translations for new messages. I don't think it will be worth it, so wouldn't make changes only to support it.

MichaelChirico · 2020-04-28T19:10:07Z

I would leave the JVM stuff to the developers working more closely with that code base. Translations only have to be updated at release time & the marginal cost from release to release would generally be low.

Anyway let's table that since you're right it's a bit premature. I'll split off the gettextf changes in a while

MichaelChirico · 2020-04-29T02:40:18Z

R/pkg/R/DataFrame.R

                    "outer", "full", "fullouter", "full_outer",
                    "left", "leftouter", "left_outer",
                    "right", "rightouter", "right_outer",
-                    "semi", "left_semi", "leftsemi", "anti", "left_anti", "leftanti")) {
+                    "semi", "left_semi", "leftsemi", "anti", "left_anti", "leftanti")


noting here that i changed this error message slightly (removed the or and aligned the ordering (leftsemi/left_semi and leftanti/left_anti are swapped vis-a-vis the %in% condition).

it may be preferable to just change the test not to be so exact. LMK

MichaelChirico · 2020-04-29T02:41:21Z

@HyukjinKwon have reverted all gettextf usage, please have another look

SparkQA · 2020-04-29T03:20:18Z

Test build #122025 has finished for PR 28365 at commit 5f36b1a.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-29T04:14:02Z

Test build #122031 has finished for PR 28365 at commit 4d86dc6.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-29T05:18:02Z

Test build #122037 has finished for PR 28365 at commit 2c92360.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-29T06:47:13Z

Test build #122040 has finished for PR 28365 at commit f278f95.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

If it's just improving the code, seems fine.

R/pkg/tests/fulltests/test_sparkSQL.R

SparkQA · 2020-04-29T19:04:50Z

Test build #122085 has finished for PR 28365 at commit 3b3c1af.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon

Looks okay but would need a closer look from me or somebody else.

R/pkg/R/DataFrame.R

HyukjinKwon · 2020-05-01T04:24:14Z

R/pkg/R/utils.R

@@ -354,8 +354,8 @@ varargsToStrEnv <- function(...) {
      } else {
        value <- pairs[[name]]
        if (!(is.logical(value) || is.numeric(value) || is.character(value) || is.null(value))) {
-          stop(paste0("Unsupported type for ", name, " : ", class(value),
-               ". Supported types are logical, numeric, character and NULL."), call. = FALSE)
+          stop("Unsupported type for ", name, " : ", toString(class(value)), ". ",


Quick question, why do we need to do toString? Were there any differences at stop(paste(..., ...)) vs stop(..., ...)?

stop uses sep='', so stop(paste0(...)) is equivalent to stop(...)

toString collapses in cases length>1, compare:

stop(toString(class(Sys.time()))) stop(class(Sys.time()))

SparkQA · 2020-05-01T08:25:39Z

Test build #122167 has finished for PR 28365 at commit 02d1728.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…ical ### What changes were proposed in this pull request? Internal usages like `{stop,warning,message}({paste,paste0,sprintf}` and `{stop,warning,message}(some_literal_string_as_variable` have been removed and replaced as appropriate. ### Why are the changes needed? CRAN policy recommends against using such constructions to build error messages, in particular because it makes the process of creating portable error messages for the package more onerous. ### Does this PR introduce any user-facing change? There may be some small grammatical changes visible in error messaging. ### How was this patch tested? Not done Closes #28365 from MichaelChirico/r-stop-paste. Authored-by: Michael Chirico <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit f53d8c6) Signed-off-by: HyukjinKwon <[email protected]>

HyukjinKwon · 2020-05-03T03:41:15Z

Merged to master and branch-3.0.

…ical ### What changes were proposed in this pull request? Internal usages like `{stop,warning,message}({paste,paste0,sprintf}` and `{stop,warning,message}(some_literal_string_as_variable` have been removed and replaced as appropriate. ### Why are the changes needed? CRAN policy recommends against using such constructions to build error messages, in particular because it makes the process of creating portable error messages for the package more onerous. ### Does this PR introduce any user-facing change? There may be some small grammatical changes visible in error messaging. ### How was this patch tested? Not done Closes apache#28365 from MichaelChirico/r-stop-paste. Authored-by: Michael Chirico <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

overhaul stop/message/warning calls to be more translation-friendly/c…

4da5ded

…anonical

probot-autolabeler bot added ML R SQL labels Apr 27, 2020

MichaelChirico commented Apr 27, 2020

View reviewed changes

R/pkg/R/client.R Outdated Show resolved Hide resolved

Michael Chirico added 2 commits April 27, 2020 16:20

some missing whitespace

b84c6f3

linting

1940eb8