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

tagGetAttribute(x, attr) returns a list if one of the values is not atomic #212

Merged
merged 23 commits into from Jul 7, 2021

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Apr 15, 2021

Could not approach by flattening and splitting values. Too many downstream dependencies reach into the attribs.

Fixes #205

To get around more complicated attribs values, a list will be returned if any value is not atomic. We still recommend that everyone use character values only (or values that can be pasted together).

@schloerke schloerke requested a review from wch April 15, 2021 18:10
@schloerke schloerke marked this pull request as ready for review April 15, 2021 18:10
R/tags.R Outdated Show resolved Hide resolved
R/tags.R Outdated Show resolved Hide resolved
R/tags.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
tests/testthat/test-tags.r Outdated Show resolved Hide resolved
Co-authored-by: Winston Chang <winston@stdout.org>
@wch
Copy link
Collaborator

wch commented Apr 16, 2021

Following up a discussion we had: We decided that we would always collapse multiple entries with the same name into a single vector/list, allowing type promotion to happen normally.

For example, div(x = "a b", x = "c") would result in an attribs data structure of c("a b", "c").

If the input includes a list or other complex data structure, like div(x = c("a", "b"), x = span("hello")), the attribs data structure would be list(c("a", "b"), span("hello")). (Note that this would generally be a bad idea to use!)

Note that we also need to be a bit careful about factors.

An additional thought I had afterward: we could add a parameter named split to tagGetAttribute() to do the following:

z <- div(x = "a b", x = "c")

tagGetAttribute(z, "x")
#> [1] "a b c"

tagGetAttribute(z, "x", split = TRUE)
#> [1] "a" "b" "c"

This would make it more convenient to check if the attribute contains a specific value, like "b".

I'm not sure the best way to handle attributes which are lists here, though.

@schloerke
Copy link
Collaborator Author

schloerke commented Apr 20, 2021

After more discussion, users are already abusing x$attribs$class. To change that structure would be very problematic.


With the latest implementation, characters are returned as normal.

z <- div(a = "x", a = "y")

z
#> <div a="x y"></div>

tagGetAttribute(z, "a")
#> [1] "x y"

But non atomic values are returned using a list structure, avoiding a call to as.character().

w <- div(a = "x", a = list(obj = TRUE))

tagGetAttribute(w, "a")
#> [[1]]
#> [1] "x"
#> 
#> [[2]]
#> [[2]]$obj
#> [1] TRUE

w
#> <div a="x TRUE"></div>

@schloerke schloerke changed the title Flatten tags attributes on tag initialization, appending tag attributes, and getting a tag attribute. tagGetAttribute(x, attr) returns a list if one of the values is not atomic Apr 20, 2021
@schloerke schloerke requested review from wch and cpsievert April 20, 2021 20:22
@schloerke schloerke marked this pull request as draft April 21, 2021 21:19
R/tag_query.R Outdated Show resolved Hide resolved
@schloerke schloerke marked this pull request as ready for review April 22, 2021 19:34
@schloerke
Copy link
Collaborator Author

I think this PR is ready to be merged.

The comment above shows a typical use case and a not-recommended use case.

The internal attrib structures are not altered and are only adjusted when printed or retrieved with tagGetAttribute()

* 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)
R/tags.R Show resolved Hide resolved
* master:
  `tagQuery()`: Remove `$print()` (#242)
  `tagQuery()`: Remove `$rebuild()`; Rebuild the pseudoRoot tag on init. (#241)
Similar behavior to printing the tag attribute
NEWS.md Outdated Show resolved Hide resolved
R/tags.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

LGTM pending minor suggestions

@cpsievert cpsievert merged commit a985267 into master Jul 7, 2021
@cpsievert cpsievert deleted the normalize_tag_attribs branch July 7, 2021 21:41
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.

Tag classes should be normalized on creation
3 participants