Skip to content

Commit

Permalink
attempted fix for #143, define special $.tbl_df method using .subset2
Browse files Browse the repository at this point in the history
  • Loading branch information
@sckott
sckott committed Jun 13, 2016
1 parent 2dcb5fa commit cdebce8
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 5 deletions.
1 change: 1 addition & 0 deletions DESCRIPTION
View file
Expand Up @@ -76,4 +76,5 @@ Collate:
'nexml_write.R'
'simmap.R'
'taxize_nexml.R'
'tbl_df.R'
RoxygenNote: 5.0.1
6 changes: 3 additions & 3 deletions R/get_characters.R
View file
Expand Up @@ -30,14 +30,14 @@
#' }
get_characters <- function(nex, rownames_as_col=FALSE, otu_id = FALSE, otus_id = FALSE){

drop = lazyeval::interp(~-matches(x), x="about|xsi.type|format")
drop = lazyeval::interp(~-dplyr::matches(x), x = "about|xsi.type|format")

otus <- get_level(nex, "otus/otu") %>%
select_(drop) %>%
dplyr::select_(drop) %>%
optional_labels(id_col = "otu")

char <- get_level(nex, "characters/format/char") %>%
select_(drop) %>%
dplyr::select_(drop) %>%
optional_labels(id_col = "char")

## Rows have otu information
Expand Down
1 change: 1 addition & 0 deletions R/tbl_df.R
View file
@@ -0,0 +1 @@
`$.tbl_df` <- function(x, i) .subset2(x, i)

This comment has been minimized.

Sign in to view

Copy link
@krlmlr

krlmlr May 16, 2017

This looks dangerous to me, because then the behavior or $.tbl_df() depends on if this package is loaded or not.

This comment has been minimized.

Sign in to view

Copy link
@sckott

sckott May 16, 2017

Author Contributor

thx for raising it, any better approach?
4 changes: 2 additions & 2 deletions man/nexml_validate.Rd
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 comments on commit cdebce8

@cboettig
Copy link
Member

Choose a reason for hiding this comment

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

Iirc this was a hack to make cran happy when tibble moved out of dplyr but wasn't on cran yet. Perhaps we can just call the tibble method directly now

@krlmlr
Copy link

@krlmlr krlmlr commented on cdebce8 May 16, 2017

Choose a reason for hiding this comment

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

You'll still get a warning if the column does not exist. If you want partial matching for column names, I'd suggest to use plain data frames. It's much safer to use the right column name.

@cboettig
Copy link
Member

Choose a reason for hiding this comment

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

@krlmlr ah right, thanks it's coming back to me now. Right, dplyr's version of tibble would return NULL if the column didn't exist, but new tibble broke this behavior and threw an error instead: tidyverse/tibble#91

We don't really want partial matching of column names, we only wanted to avoid the hard error. Based on tidyverse/tibble#91 (comment) it looks like we can now drop this line and just use a supressWarnings, since iiuc tibble only warns (and returns NULL) instead of throwing an error when no match is found(?)

Please sign in to comment.