nealrichardson commented on a change in pull request #11324:
URL: https://github.com/apache/arrow/pull/11324#discussion_r727352871
##########
File path: r/R/field.R
##########
@@ -54,11 +54,11 @@ Field <- R6Class("Field",
}
)
)
-Field$create <- function(name, type, metadata) {
+Field$create <- function(name, type, metadata, nullable = TRUE) {
Review comment:
Judging from the PR diff, it looks like you need to upgrade roxygen2 to
the latest version (which adds the `@examplesIf` tag) and then re-render the
docs.
##########
File path: r/tests/testthat/test-schema.R
##########
@@ -38,6 +38,20 @@ test_that("Schema print method", {
)
})
+test_that("Schema with non-nullable fields", {
+ expect_output(
+ print(schema(b = double(), c = list(bool(), NULL, FALSE), d = string())),
Review comment:
I'm not a fan of the unnamed list of args, particularly with one that is
ignored. What if instead the interface supported was:
```
schema(field("b", double()), field("c", bool(), nullable = FALSE))
```
i.e. schema() accepts either a list of Fields or a named list of DataTypes
(which get turned into Fields, as it currently works)
##########
File path: r/R/schema.R
##########
@@ -136,7 +136,16 @@ Schema <- R6Class("Schema",
}
)
)
-Schema$create <- function(...) schema_(.fields(list2(...)))
+Schema$create <- function(...) {
+
Review comment:
```suggestion
```
##########
File path: r/R/schema.R
##########
@@ -136,7 +136,16 @@ Schema <- R6Class("Schema",
}
)
)
-Schema$create <- function(...) schema_(.fields(list2(...)))
+Schema$create <- function(...) {
+
+ .list <- list2(...)
+ if (all(sapply(.list, function(x) inherits(x = x, what = "Field")))) {
Review comment:
Since we're already using `purrr` in this package we can make this a
little more concise (and type stable)
```suggestion
if (all(map_lgl(.list, ~ inherits(. "Field")))) {
```
##########
File path: r/R/schema.R
##########
@@ -136,7 +136,16 @@ Schema <- R6Class("Schema",
}
)
)
-Schema$create <- function(...) schema_(.fields(list2(...)))
+Schema$create <- function(...) {
+
+ .list <- list2(...)
+ if (all(sapply(.list, function(x) inherits(x = x, what = "Field")))) {
+ schema_(.list)
+ } else {
+ schema_(.fields(.list))
+ }
+
Review comment:
```suggestion
```
##########
File path: r/R/schema.R
##########
@@ -136,7 +136,14 @@ Schema <- R6Class("Schema",
}
)
)
-Schema$create <- function(...) schema_(.fields(list2(...)))
+Schema$create <- function(...) {
+ .list <- list2(...)
+ if (all(map_lgl(.list, ~ inherits(. "Field")))) {
Review comment:
oops
```suggestion
if (all(map_lgl(.list, ~ inherits(., "Field")))) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
