remove extra forward slashes - #11
Conversation
hpages
left a comment
hpages
left a comment
There was a problem hiding this comment.
Thanks @Artur-man. See inline comments.
| expect_true(is(sink, "ZarrRealizationSink")) | ||
| expect_true(is(sink, "RealizationSink")) | ||
| format <- ZarrArray:::get_zarr_format(paste0(sink@zarr_path, "/")) | ||
| format <- ZarrArray:::get_zarr_format(sink@zarr_path) |
There was a problem hiding this comment.
get_zarr_format() is a thin wrapper to Rarr:::.read_array_metadata() and IIRC earlier versions of the latter required the trailing slash. Happy to get rid of it.
| if (startsWith(name, "/")) { | ||
| name <- sub("^/*", "/", name) # only keep first leading slash | ||
| } else { | ||
| name <- paste0("/", name) |
There was a problem hiding this comment.
The purpose of normarg_zarr_group() is to normalize the supplied group, but the proposed change no longer does that. I'm open to discuss whether the normalized group should have a leading slash or not. In the current normarg_zarr_group(), the decision was made to always have a leading slash, whether the user supplied it or not. However, the proposed change no longer makes any decision with respect to the leading slash: it keeps it if the user supplied it but it doesn't add it if the user didn't. So it doesn't normalize the group anymore.
Personally I have a small preference for having the leading slash in the normalized group. This is consistent with what we do with the new group() method in HDF5Array which in turn is consistent with what they do in rhdf5::h5ls(). More generally speaking I think it's important to try to keep things as consistent as possible across HDF5Array and ZarrArray.
BTW, I'm about to add a group() method in ZarrArray that will return the normalized group of a ZarrSparseMatrixSeed or ZarrSparseMatrix object.
| if (!zarr_exists(zarr_store, group)) | ||
| stop(wmsg("Group \"", group, "\" does not exist in this Zarr store")) | ||
| if (zarr_node_is_dataset(zarr_store, group)) { | ||
| is_X_or_layer <- group == "/X" || startsWith(group, "/layers/") |
There was a problem hiding this comment.
This is a good example of why normalization is important. If we know that group is guaranteed to always start with a single /, then the current code does the right thing. If we know that group is guaranteed to never start with a /, then the current code no longer does the right thing but is easy to fix.
But if group sometimes has the leading slash and sometimes not, then both, the current code and the proposed change are wrong.
Additionally, the proposed change is wrong for another reason: this code tries to recognize datasets that belong to the /layers group (in which case the dataset names are going to be of the form /layers/<dataset_name> e.g. /layers/counts), but the proposed change will recognize datasets that belong to groups that start with layers (e.g. layersalt), which is not what we want.
2 participants
Hi @hpages, following @Bisaloo's suggestion I simplified group names since we have
file.paths instead ofpaste0s now. Are these ok?