Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow trailing commas in more places #3328

Merged
merged 6 commits into from
Mar 23, 2021
Merged

Allow trailing commas in more places #3328

merged 6 commits into from
Mar 23, 2021

Conversation

hadley
Copy link
Member

@hadley hadley commented Mar 9, 2021

I grepped for list(...) and replaced with rlang::list2(...). This also enables !!! which is generally not important for Shiny because it automatically splices lists/tagLists, but I doubt it will affect any existing code.

Let me know if you want me to document this, or add a news bullet.

I grepped for list(...) and replaced with rlang::list2(...). This also enables !!! which is generally not important for Shiny because it automatically splices lists/tagLists, but I doubt it will affect any existing code.
@hadley hadley requested a review from wch March 9, 2021 13:35
@cpsievert cpsievert added this to the 1.7 milestone Mar 12, 2021
@cpsievert
Copy link
Collaborator

Since there was some overlap with messaging for #3315 I took a stab at a NEWS item (and fixed the merge conflict).

NEWS.md Outdated
@@ -9,6 +9,8 @@ shiny 1.6.0.9000

### New features and improvements

* All uses of `list(...)` have been replaced with `rlang::list2(...)`. By effect, `{rlang}`'s `!!!` operator may now be used to "splice" a list of argument values into `...`. We think this'll be particularly useful for passing a list of `tabPanel()` to their consumers (i.e., `tabsetPanel()`, `navbarPage()`, etc). For example, `tabs <- list(tabPanel("A", "a"), tabPanel("B", "b")); navbarPage(!!!tabs)`. (#3315 and #3328)
Copy link
Member Author

@hadley hadley Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* All uses of `list(...)` have been replaced with `rlang::list2(...)`. By effect, `{rlang}`'s `!!!` operator may now be used to "splice" a list of argument values into `...`. We think this'll be particularly useful for passing a list of `tabPanel()` to their consumers (i.e., `tabsetPanel()`, `navbarPage()`, etc). For example, `tabs <- list(tabPanel("A", "a"), tabPanel("B", "b")); navbarPage(!!!tabs)`. (#3315 and #3328)
* All uses of `list(...)` have been replaced with `rlang::list2(...)`. This means that you can use trailing `,` without error and use rlang's `!!!` operator to "splice" a list of argument values into `...`. We think this'll be particularly useful for passing a list of `tabPanel()` to their consumers (i.e., `tabsetPanel()`, `navbarPage()`, etc). For example, `tabs <- list(tabPanel("A", "a"), tabPanel("B", "b")); navbarPage(!!!tabs)`. (#3315 and #3328)

Is that true about splicing though? I thought tagList() already did auto-splicing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I believe all of htmltools uses dots_list() now. I couldn't find any mention of it in the NEWS though so I figured it wouldn't hurt to say it just generally works now with any ...?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't the suggestion be without error instead of with out warning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops yeah. But I meant doesn't splicing already happen without !!!?

Copy link
Collaborator

@cpsievert cpsievert Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, technically no, but in practice the difference usually isn't important (at least as far as renderTags() is concerned)

> str(tagList(list(tags$a(), tags$div())))
List of 1
 $ :List of 2
  ..$ :List of 3
  .. ..$ name    : chr "a"
  .. ..$ attribs : Named list()
  .. ..$ children: list()
  .. ..- attr(*, "class")= chr "shiny.tag"
  ..$ :List of 3
  .. ..$ name    : chr "div"
  .. ..$ attribs : Named list()
  .. ..$ children: list()
  .. ..- attr(*, "class")= chr "shiny.tag"
 - attr(*, "class")= chr [1:2] "shiny.tag.list" "list"
> str(tagList(!!!list(tags$a(), tags$div())))
List of 2
 $ :List of 3
  ..$ name    : chr "a"
  ..$ attribs : Named list()
  ..$ children: list()
  ..- attr(*, "class")= chr "shiny.tag"
 $ :List of 3
  ..$ name    : chr "div"
  ..$ attribs : Named list()
  ..$ children: list()
  ..- attr(*, "class")= chr "shiny.tag"
 - attr(*, "class")= chr [1:2] "shiny.tag.list" "list"

Co-authored-by: Hadley Wickham <[email protected]>
@@ -9,6 +9,8 @@ shiny 1.6.0.9000

### New features and improvements

* All uses of `list(...)` have been replaced with `rlang::list2(...)`. This means that you can use trailing `,` without error and use rlang's `!!!` operator to "splice" a list of argument values into `...`. We think this'll be particularly useful for passing a list of `tabPanel()` to their consumers (i.e., `tabsetPanel()`, `navbarPage()`, etc). For example, `tabs <- list(tabPanel("A", "a"), tabPanel("B", "b")); navbarPage(!!!tabs)`. (#3315 and #3328)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* All uses of `list(...)` have been replaced with `rlang::list2(...)`. This means that you can use trailing `,` without error and use rlang's `!!!` operator to "splice" a list of argument values into `...`. We think this'll be particularly useful for passing a list of `tabPanel()` to their consumers (i.e., `tabsetPanel()`, `navbarPage()`, etc). For example, `tabs <- list(tabPanel("A", "a"), tabPanel("B", "b")); navbarPage(!!!tabs)`. (#3315 and #3328)
* All uses of `list(...)` have been replaced with `rlang::list2(...)`. This means that you can use trailing `,` without error and use rlang's `!!!` operator to "splice" a list of argument values into `...`. (#3315 and #3328)

Since it doesn't provide any new benefit, I think it's better to not over advertise it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!! inside tabsetPanel() is new (was merged earlier today), but !!! inside tagList() isn't.

That said, I'd be OK with removing it, but I'll leave that up to @wch

@cpsievert cpsievert merged commit e29d92c into master Mar 23, 2021
@cpsievert cpsievert deleted the trailing-commas branch March 23, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants