-
Notifications
You must be signed in to change notification settings - Fork 18
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
Safer check for NAs in tiledb_config()
#519
Conversation
Replace `if(is.na())` as R 4.2 errors when conditions > 1L Instead, removes NAs from `config` Then, checks the length of `config`; if there is a length, adds it to the TileDB config fixes #518
This looks good. It may be a function that predates me, and the use if NA here is a little non-standard (maybe check for |
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.
Looks good especially with the added tests.
|
Let's table whether |
The other reason that issue went undetected is that most uses of config use this pattern for one or more settings cfg <- tiledb_config()
cfg["vfs.s3.region"] <- "us-west-1"
ctx <- tiledb_ctx(cfg) and the more compact form of the named charecter vector which, as you correctly identified could be longer than length one, is not use much (but even more convenient). |
Oh, one more thing: can you add a line to NEWS.md with the PR number? I found adding And then as it is approved feel free to squash-merge. |
[ci_skip]
@@ -1,6 +1,7 @@ | |||
# tiledb 0.18.0.2 (Development) | |||
|
|||
* This release of the R package builds against [TileDB 2.14.1](https://github.com/TileDB-Inc/TileDB/releases/tag/2.14.1), and has also been tested against earlier releases as well as the development version (#502). | |||
* Safer checking of `NAs` in `tiledb_config()` to support R 4.2 conditional lengths (#518, #519) |
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.
Thanks for updating the file. Per precedent in this file (as well as what Core does) I tend to only list the PRs, not issues. We can address that in the next edition, no rush.
Replace
if(is.na())
as R 4.2 errors when conditions > 1L Instead, removes NAs fromconfig
Then, checks the length of
config
; if there is a length, adds it to the TileDB configfixes #518