Skip to content
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

ZarrArray: add a method to get the DimensionSeparator #32

Conversation

melissalinkert
Copy link

3a4e7dd adds a simple getter to ZarrArray to retrieve the DimensionSeparator. It also updates the dimension separator test to use the new getter.

When updating the dimension separator test, I noticed that some of the tests were not being run. b4eed70 fixes it so that every test class is run.

Without this change, several of the test classes were not run because the class
name did not match the default Surefire naming:

https://maven.apache.org/surefire-archives/surefire-3.0.0-M5/maven-surefire-plugin/examples/inclusion-exclusion.html

ZarrArrayTest_dimensionSeparator is one example of an affected test.
This allows the array's `DimensionSeparator` to be easily checked.

The corresponding test is also updated to use the new getter instead of
reflective access.
@SabineEmbacher
Copy link
Collaborator

Dear Melissa,

many thanks for your pull request!

I would like to keep the public API as small as possible.
Are you able to describe a real use case where a getter for dimension separator is needed?

Of course then the test is easier to implement and easier to understand, but is there a real use case, really a need for such a public getter method?

One of my API design principles is to implement an API along real use cases, not to simplify tests.

So I'm very excited to see a presentation of a corresponding use case.
This is always a great opportunity for me to learn and grow through it.

I look forward to receive your reply.

Best Regards
Sabine

@SabineEmbacher SabineEmbacher changed the base branch from master to getter-for-dimensionseparator April 28, 2022 06:22
@SabineEmbacher SabineEmbacher merged commit 094a9d1 into bcdev:getter-for-dimensionseparator Apr 28, 2022
@SabineEmbacher
Copy link
Collaborator

Many thanks for this pull request!

@melissalinkert
Copy link
Author

@SabineEmbacher: thank you for considering these changes.

My immediate use case is to be able to directly test that a ZarrArray in a newly written Zarr dataset has the correct dimension separator, for example here:

https://github.com/glencoesoftware/bioformats2raw/pull/148/files#diff-3bf686322477afd2500c0b42697eb037dd07ae3fc65e17ece2facd25bf5c6081R247

I understand that is still just simplifying tests, but as a downstream API consumer it is much easier to be able to call a getter when needed rather than using reflection to access a private field or having to infer the dimension separator based upon which files exist on disk.

I appreciate the goal of keeping the API small, but getDimensionSeparator() as proposed here would be useful outside of the jzarr tests and is consistent with the other getters that are already in ZarrArray.

@melissalinkert
Copy link
Author

@SabineEmbacher : I see this pull request was merged, but not in the master branch. Are these changes still being considered for inclusion in the master branch? Do you need any additional information from me?

@SabineEmbacher
Copy link
Collaborator

@melissalinkert
I still want to talk to my work colleagues and direct jzarr users about this.
I have also noticed inconsistencies in older code that was written by myself a long time ago.
Unfortunately, I am very busy at the moment.

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.

2 participants