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

tagQuery(): Rename$root() to $allTags(), $selected() to $selectedTags(); Print $selectedTags() like a list() #230

Merged
merged 15 commits into from
May 4, 2021

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Apr 21, 2021

  • @wch, should we adjust the print method of tagList() to display [[i]] like a regular list?
    • If so, we can remove visibleTagList()
tagQuery(div(span(), a()))$root()
#> <div>
#>   <span></span>
#>   <a></a>
#> </div>

tagQuery(div(span(), a()))$selected()
#> [[1]]
#> <div>
#>   <span></span>
#>   <a></a>
#> </div>

# has extra `visible` class
tagQuery(div(span(), a()))$selected() %>% str()
#> List of 1
#>  $ :List of 3
#>   ..$ attribs : Named list()
#>   ..$ name    : chr "div"
#>   ..$ children:List of 2
#>   .. ..$ :List of 3
#>   .. .. ..$ attribs : Named list()
#>   .. .. ..$ name    : chr "span"
#>   .. .. ..$ children: list()
#>   .. .. ..- attr(*, "class")= chr "shiny.tag"
#>   .. ..$ :List of 3
#>   .. .. ..$ attribs : Named list()
#>   .. .. ..$ name    : chr "a"
#>   .. .. ..$ children: list()
#>   .. .. ..- attr(*, "class")= chr "shiny.tag"
#>   ..- attr(*, "class")= chr "shiny.tag"
#>  - attr(*, "class")= chr [1:3] "htmltools.tag.list.visible" "shiny.tag.list" "list"

tagQuery(div(span(), a()))$root() %>% format() %>% cat()
#> <div>
#>   <span></span>
#>   <a></a>
#> </div>

@schloerke schloerke requested a review from cpsievert April 21, 2021 21:15
* master:
  Provide better error message when adding unnamed attrib values (#229)
R/tag_query.R Outdated Show resolved Hide resolved
* master:
  Allow tag query `$*Class()` methods to no-op on length 0 inputs (#236)
@schloerke schloerke changed the title Tag query $root() now returns a visibleTagList() and $selected() also returns a visibleTagList() Tag query $root() now always returns atagList() and $selected() always returns a visibleTagList() Apr 22, 2021
@wch
Copy link
Collaborator

wch commented Apr 27, 2021

Why do we want to always return a tagList? I do see that there's an advantage in that the return type is more stable, but on the other hand, in most cases, the input is a single tag object at the top level, and the user would expect the returned object to also be a tag (instead of tagList).

@schloerke
Copy link
Collaborator Author

Thought process was "anything in gives a tagList out. Then the same tagList in gives a tagList out." After one cycle, the input and output objects are stable.

I'm ok putting it back to the flexible return type knowing that it is not stable as I would believe that few people will call tagList(myTag)$append(otherTag)$root() and not be shocked by the return type changing.

@wch
Copy link
Collaborator

wch commented May 3, 2021

I think that it's OK to return a variable type. If anything, I think always returning a tagList will cause more confusion, because in most cases, that would mean that what comes out is different from what goes in. If we really wanted, we could add a parameter to $root() which tells it to always return a tagList, and have it default to FALSE.

Also, as I was looking at this PR and rstudio/shiny#3372, I think that name $root() is a bit confusing. It sort of suggests you're just getting the root node, but you're really getting the whole tree. Not sure of a better alternative at this moment, though.

@schloerke
Copy link
Collaborator Author

@wch

I think that name $root() is a bit confusing

  • $dom()?
  • $topLevelTags()?
  • $asTags()?

$html() is too related to .html() which has the description of ...

Get the HTML contents of the first element in the set of matched elements.

... so my gut reaction is not comfortable using $html() due to the differences in implementation.

R/tag_query.R Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented May 3, 2021

Some possible names:

  • allTags()
  • asTags(). This might be a little confusing because users may expect it to just return the selected items. Maybe $selected() should also then be renamed to $asTagsSelected() or $selectedAsTags()? That one doesn't need a short, concise name because it will not be commonly used, I think.

@schloerke
Copy link
Collaborator Author

How about $allTags() and $selectedTags()?

@schloerke schloerke changed the title Tag query $root() now always returns atagList() and $selected() always returns a visibleTagList() Tag query $root() now always returns atagList() and $selected() always returns a tagList() that prints like a list May 3, 2021
@wch
Copy link
Collaborator

wch commented May 3, 2021

$allTags() and $selectedTags() works for me. @cpsievert?

We should also have a version bump because of the new names.

@schloerke schloerke changed the title Tag query $root() now always returns atagList() and $selected() always returns a tagList() that prints like a list tagQuery(): Rename$root() to $allTags(), $selected() to $selectedTags(); Print $selectedTags() like a list() May 3, 2021
@schloerke schloerke requested review from cpsievert and wch May 3, 2021 22:33
@cpsievert
Copy link
Collaborator

LGTM 👍

R/tags.R Show resolved Hide resolved
@schloerke schloerke merged commit 02d1ae7 into master May 4, 2021
@schloerke schloerke deleted the tag_query_tweaks branch May 4, 2021 14:52
schloerke added a commit that referenced this pull request May 4, 2021
* master:
  `tagQuery()`: Rename`$root()` to `$allTags()`, `$selected()` to `$selectedTags()`; Print `$selectedTags()` like a `list()` (#230)
  tagQuery(): Rebuild less often and do not check for tag env cycles; Rename `$reset()` -> `$resetSelected()` (#235)
  Bump rlang dev version
  Revert "Return invisibly when not creating a new tagQuery() object (#228)"
  Allow tag query `$*Class()` methods to no-op on length 0 inputs (#236)
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