selectInput: improve factor handling

[alandipert]

This work was motivated by #2515, which demonstrated that unnamed factors work as expected, but named factors did not.

1. listify() was restructured in order to make clearer how various types are handled. Support for named factors was added.
2. selectInput() documentation now mentions factor handling.
3. Additional unit tests were added to check factor handling, and existing unit tests were improved to exercise handling of complex and logical vectors.

Open Questions

1. Should selectInput() produce a warning if a factor is used as an option list or otherwise included in the list of options? We decided not to produce a warning.

QA Notes

Given this test application:

library(shiny)

choices <- factor(setNames(letters, LETTERS))
ui <- fluidPage(selectInput("example", "Choose", choices))
server <- function(input, output) {}
shinyApp(ui = ui, server = server)

• On Shiny 1.3.2 (the latest released version on CRAN), running the test app should produce an error like the following: Error in (function (choice, name) : All sub-lists in "choices" must be named.
• Install Shiny from Github: devtools::install_github("rstudio/shiny") (this PR has been merged)
• Run the test app again, you should see no error
• The test app should display a selectInput that looks and works like the one below

[schloerke]

... else ensureNames(as.list(x)) ?

Is the goal to return a list with every element's name as '' or to return a list with no names?

[alandipert]

In that case, return a list with every element's name as '' (unless it's already named)

[schloerke]

That's my interpretation of https://github.com/rstudio/shiny/pull/2524/files#diff-c851d180beb8024d6b46a73dab4756d9L113 . makeNamed(as.list(val)) vs as.list(ensureNamed(x))

[schloerke]

My gut reaction is this should be rewritten to be a if / else.

• It wouldn't need to be exported to work.
• It could handle odd s3 cases better (things that don't have atomic classes).

Setup:

if `is.list`, `is.null`
  "code for `is.list`"
else if `is.numeric`, `is.complex`, `is.logical`, `is.factor`
  "code for `is.numeric`"
else 
  "code for `is.character`"

[alandipert]

You know, now that it has all the @export noise, I have to agree. I swear, it was looking pretty tight before all that 😄

[wch]

Since the listify generic isn't exported from the package, you don't need to @export every method, and therefore nothing will be added to the NAMESPACE file. That said, I also lean toward having a single function with if-else here.

[schloerke]

I would add {``} due to the nested if/else statement.

(I like them any time there is anything beyond an if. Makes it so things can be executed in the console.)

[schloerke]

Fix travis. otherwise LGTM!

[wch]

One thought -- we should make sure there are tests for lists (and atomic vectors) where the list is named, but some or all of the names are "". For example:

x <- list(A="a", "b", "c")
x
#> $A
#> [1] "a"
#> 
#> [[2]]
#> [1] "b"
#> 
#> [[3]]
#> [1] "c"
attributes(x)
#> $names
#> [1] "A" ""  ""

y <- x[2:3]
y
#> [[1]]
#> [1] "b"
#> 
#> [[2]]
#> [1] "c"
attributes(y)
#> $names
#> [1] "" ""

# Contrast y with:
z <- list("b", "c")
z
#> [[1]]
#> [1] "b"
#> 
#> [[2]]
#> [1] "c"
attributes(z)
#> NULL

[alandipert]

@wch there's a test for that case:

shiny/tests/testthat/test-bootstrap.r

Line 89 in 0d7b149

choicesWithNames(c(A=Inf+0i, 1+0i, C=0+0i)),

Confirmed is good on Slack

[wch]

Looks good! Just a couple of small nits.