Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4431 +/- ##
==========================================
- Coverage 85.43% 85.21% -0.22%
==========================================
Files 136 136
Lines 21437 21434 -3
==========================================
- Hits 18314 18265 -49
- Misses 3123 3169 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| soma_domain <- domain | ||
| if (is.null(soma_domain)) { | ||
| .deprecate( | ||
| lifecycle::deprecate_stop( |
There was a problem hiding this comment.
This should not need to be changed: .deprecate() will automatically signal a defunct stage with the next release
| if ( | ||
| !(is.null(soma_domain) || .is_domain(soma_domain, index_column_names)) | ||
| ) { |
There was a problem hiding this comment.
We can simplify the logic here as domain/soma_domain can no longer be NULL
| if ( | |
| !(is.null(soma_domain) || .is_domain(soma_domain, index_column_names)) | |
| ) { | |
| if (!.is_domain(soma_domain, index_column_names)) { |
| ## needed eg after open() to set (Arrow) type | ||
| #' @description Sets a cache value for the datatype. | ||
| #' | ||
| #' @section Lifecycle: | ||
| #' As of \pkg{tiledbsoma} 2.1.0, \code{$set_data_type()} is deprecated; this | ||
| #' functionality is no longer needed as \code{libtiledbsoma} now accurately | ||
| #' sets the TileDB data type upon array opening | ||
| #' | ||
| #' @param type A character value describing the TileDB data type. | ||
| #' | ||
| set_data_type = function(type) { | ||
| .deprecate( | ||
| when = "2.1.0", | ||
| what = sprintf("%s$set_data_type()", class(self)[1L]) | ||
| ) | ||
| soma_debug(sprintf( | ||
| "[SOMANDArrayBase::set_data_type] caching type %s", | ||
| type$ToString() | ||
| )) | ||
| private$.type <- type | ||
| }, | ||
|
|
||
| #' @description Test if the array has the upgraded resizeable shape feature |
There was a problem hiding this comment.
If I understand the deprecation policy correctly, shouldn't this be removed as part of tiledbsoma 4.x? Or have we already reached defunct, allowing us to remove this as part of tiledbsoma 3.x? (or am I misunderstanding the deprecation policy altogether?)
| private$.read_only_error("tiledbsoma_ctx") | ||
| } | ||
| .deprecate( | ||
| lifecycle::deprecate_stop( |
There was a problem hiding this comment.
.deprecate() should be able to switch this to a defunct error rather than a deprecation warning automatically with tiledbsoma 2.5.0 or 3.x
| if (!is.null(tiledbsoma_ctx)) { | ||
| .deprecate( | ||
| if (lifecycle::is_present(tiledbsoma_ctx)) { | ||
| lifecycle::deprecate_stop( |
There was a problem hiding this comment.
.deprecate() should be able to switch this to a defunct error rather than a deprecation warning automatically with tiledbsoma 2.5.0 or 3.x
| if (lifecycle::is_present(tiledbsoma_ctx)) { | ||
| lifecycle::deprecate_stop( | ||
| what = "DimReduc.write_soma(tiledbsoma_ctx)", | ||
| when = "2.3.0", | ||
| details = "Use `context` instead." | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'm confused about this (and the other write_soma(tiledbsoma_ctx) defunct signals):
- the
whatimplies there's awrite_somamethod for theDimReducgeneric (eg.DimReduc(tiledbsoma_ctx = )); thiswhatshould bewrite_soma.DimReduc(tiledbsoma_ctx)orwrite_soma(tiledbsoma_ctx)aswrite_soma.DimReduc()will never be called directly - we could simplify all these defunct signals by adding the call to
.deprecate()orlifecycle::deprecate_stop()in the genericwrite_soma()and continuing to passtiledbsoma_ctx = tiledbsoma_ctxto all subsequentwrite_soma()calls
| test_that("SOMAContext from default SOMATileDBContext", { | ||
| # Create context. | ||
| with_mocked_bindings( | ||
| .tiledbsoma_deprecation_version = function() "2.3.0", | ||
| .deprecation_stage = function(when) "deprecate", | ||
| { | ||
| lifecycle::expect_deprecated(tiledbsoma_ctx <- SOMATileDBContext$new()) | ||
| lifecycle::expect_deprecated(context <- get_soma_context(NULL, tiledbsoma_ctx, what="get_soma_context()")) | ||
| } | ||
| ) |
There was a problem hiding this comment.
As per the deprecation policy, can we keep and modify some of these tests to ensure that the defunct error is being signalled properly?
2 participants
Issue and/or context: Closes SOMA-815
Changes:
R:
tiledbsoma_ctxparameter to be defunct.NULL.set_data_type.Pythons:
None.Notes for Reviewer: