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

Add support for hanging commas #135

Merged
merged 4 commits into from
Aug 19, 2019
Merged

Add support for hanging commas #135

merged 4 commits into from
Aug 19, 2019

Conversation

trestletech
Copy link
Contributor

Closes #128

Uses rlang::dots_list instead of list to support trailing commas with missing arguments a la

tagList("hi", )
div("one", )

I'll confess that I was guessing at which of the expectations in test-deps were going to turn out to be lists or dot_lists. That makes me wary that I might have missed something or that this may be an incomplete migration.

This does change the hash for singletons, but I added a test to confirm that the old hash will still work.

Testing notes

Add a hanging comma at the end of some tags when defining a Shiny UI and it should work e.g.

tagList("hi", )
div("one", )

Multiple trailing commas should throw an error:

div("one", , )

Non-trailing commas should throw an error:

div( ,"one", )

Make explicit that multiple hanging commas fails.

Add test for non-trailing
@trestletech trestletech requested a review from wch August 13, 2019 16:05
NEWS Outdated
@@ -1,6 +1,8 @@
htmltools 0.3.6.9003
--------------------------------------------------------------------------------

* Add support for trailing commas in tagLists and the predefined tags. (#128)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The format is usually:

Fixed #128: .... (#135)

I'll just make the change directly.

@wch wch merged commit b5b1a6e into master Aug 19, 2019
@wch wch deleted the jeff/hanging-commas branch August 19, 2019 15:40
@hadley
Copy link
Member

hadley commented Aug 20, 2019

I’d suggest using list2() here, and I don’t think you should need to change the tests.

@jcheng5
Copy link
Member

jcheng5 commented Aug 20, 2019

(sorry just noticed this PR now)

This gives HTML tag functions tidyeval semantics now, doesn't it? Is there any situation where that could break existing code? (Maybe where an expression being passed to an HTML tag includes dplyr calls that need to interpret the !! themselves?)

Even if there are such cases, depending on how edgy they might be, it might not be a showstopper anyway. But the fear of such breaking changes was what stopped me from immediately making this change, hoping we might find or make a variant of list2 that didn't do tidyeval--or else convince ourselves that the edge cases are vanishingly edgy.

@hadley
Copy link
Member

hadley commented Aug 20, 2019

Not general tidy evaluation semantics, just tidy dots semantics (i.e. you can use !!!). (i.e. I think it's very unlikely that this will change semantics in practice; switching from list() to list2() is something that we generally do without worry)

@jcheng5
Copy link
Member

jcheng5 commented Aug 20, 2019

Thanks @hadley that's perfect

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.

Ignore trailing commas
4 participants