-
Notifications
You must be signed in to change notification settings - Fork 25
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
[r] Write group-level string metadata as TILEDB_STRING_UTF8
#3469
[r] Write group-level string metadata as TILEDB_STRING_UTF8
#3469
Conversation
String group-level metadata was previously encoded using `TILEDB_CHAR` or `TILEDB_STRING_ASCII`; however, this resulting in the metadata being read in as `bytes` in the Python API instead of as `str`. The Python API already [encodes all strings (`str`) as `TILEDB_STRING_UTF8`](https://github.com/single-cell-data/TileDB-SOMA/blob/884342a1ceb994d677c52c74ba2d789fc4e208d4/apis/python/src/tiledbsoma/common.cc#L211-L223) so this PR brings the R API in-line with the Python API [SC-61001](https://app.shortcut.com/tiledb-inc/story/61001) resolves #2698
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The facts that no unit tests have been added -- and that no existing unit-test cases are now failing -- are reassuring confirmation that we do need the cross-language tests here #2698 to clinch the typing compatibilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a test run regularly - is that something for this PR, or follow-up backlog?
@bkmartinjr please see
@mojaveazure the closing criterion for #2698 is that cross-language test |
372ffb8
to
bb678d6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3469 +/- ##
=======================================
Coverage 86.32% 86.32%
=======================================
Files 55 55
Lines 6339 6339
=======================================
Hits 5472 5472
Misses 867 867
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@bkmartinjr @johnkerl round-trip checks added in 941745f |
[ci skip]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mojaveazure awesome, thank you!
Bump develop version [ci skip]
String group-level metadata was previously encoded using
TILEDB_CHAR
orTILEDB_STRING_ASCII
; however, this resulting in the metadata being read in asbytes
in the Python API instead of asstr
. The Python API already encodes all strings (str
) asTILEDB_STRING_UTF8
so this PR brings the R API in-line with the Python APISC-61001
adresses #2698