[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r918440696
##
R/pkg/tests/fulltests/test_context.R:
##
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat
unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as
masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
cc @falaki @felixcheung @shivaram FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917898330
##
R/pkg/tests/fulltests/test_context.R:
##
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat
unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as
masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Let's use a different name for now. e.g., `getFunc` and explain it in the
docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917892592
##
R/pkg/tests/fulltests/test_context.R:
##
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat
unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as
masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Ah .. even with that the signature would likely conflict. hm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37135: [SPARK-39723][R] Implement functionExists/getFunction in SparkR for 3L namespace
HyukjinKwon commented on code in PR #37135:
URL: https://github.com/apache/spark/pull/37135#discussion_r917891535
##
R/pkg/tests/fulltests/test_context.R:
##
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat
unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as
masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Should probably we should define this with `setMethod` (see `functions.R`)
to avoid the complete masking.
##
R/pkg/tests/fulltests/test_context.R:
##
@@ -21,10 +21,11 @@ test_that("Check masked functions", {
# Check that we are not masking any new function from base, stats, testthat
unexpectedly
# NOTE: We should avoid adding entries to *namesOfMaskedCompletely* as
masked functions make it
# hard for users to use base R functions. Please check when in doubt.
- namesOfMaskedCompletely <- c("cov", "filter", "sample", "not")
+ namesOfMaskedCompletely <- c("cov", "filter", "sample", "not", "getFunction")
Review Comment:
Should probably we should define this with `setMethod` with a signature (see
`functions.R`) to avoid the complete masking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
