Skip to content

Export tiledb_datatype_R_type#336

Merged
eddelbuettel merged 1 commit intomasterfrom
feature/sc-12830/export_datatype_mapper
Dec 14, 2021
Merged

Export tiledb_datatype_R_type#336
eddelbuettel merged 1 commit intomasterfrom
feature/sc-12830/export_datatype_mapper

Conversation

@eddelbuettel
Copy link
Copy Markdown
Contributor

This is a bit of a micro-PR to benefit @LTLA and BioConductor package TileDBArray which currently accesses a C++-level function tiledb_datatype_R_type mapping from TileDB type (as string) to R type (as string) via ::: as the function is not exported.

This PR corrects this and exports the function.

@shortcut-integration
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

I believe the mapping here is stable and (effectively) covered by existing tests, since we use this function internally, right?

@eddelbuettel
Copy link
Copy Markdown
Contributor Author

eddelbuettel commented Dec 13, 2021

Yes it should be and the package itself does not use it outside of code disappearing with the to-be-deprecated-in-draft-PR-#335:

edd@rob:~/git/tiledb-r(feature/sc-12830/export_datatype_mapper)$ ag tiledb_datatype_R_type R/
R/SparseArray.R
116:    type <- tiledb_datatype_R_type(dtype)

R/DenseArray.R
231:    type <- tiledb_datatype_R_type(dtype)

R/RcppExports.R
51:tiledb_datatype_R_type <- function(datatype) {
52:    .Call(`_tiledb_tiledb_datatype_R_type`, datatype)
edd@rob:~/git/tiledb-r(feature/sc-12830/export_datatype_mapper)$ 

So it really is just a courtesy export for external functionality shadowing what we used to do / still do (technically) til the deprecated code is lifted. But it does no harm and is, as you say, type-stable.

@eddelbuettel eddelbuettel merged commit 93690ac into master Dec 14, 2021
@eddelbuettel eddelbuettel deleted the feature/sc-12830/export_datatype_mapper branch December 14, 2021 17:33
@eddelbuettel eddelbuettel mentioned this pull request Dec 17, 2021
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