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.

Sign up for GitHub

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

Relax version detection #189

Merged
merged 1 commit into from
Apr 18, 2022
+5 −0
Merged

Relax version detection #189

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ome_zarr/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
def format_from_version(version: str) -> "Format":

for fmt in format_implementations():

# Support floating-point versions like `0.2`
if isinstance(version, float):
version = str(version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is primarily consumed at the reader level currently and hence quite downstream of the generation process, this raises the question of whether this library should also raise a WARN level logging statement indicating the mismatch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, though I'm already annoyed by the format mismatch warnings. Perhaps we could differentiate between a more purely user (view()) and developer (validate()) context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite annoyed by the format mismatch warning as well - happy to create an issue or even look into it 😄

In this scenario, the the warning is about a core issue with the data validation rather than exposing the internal guessing logic of the library so assuming I had to pick between the two, I would personally only keep the former. The idea of differentiating the usage contexts is interesting and possibly brings us back to the validation flag that @will-moore introduced while working on #189.

Copy link
Member Author

@joshmoore joshmoore Apr 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found why it's duplicated and will open a PR (#190). We'll still need to decide on the first mismatch.


if fmt.version == version:
return fmt
raise ValueError(f"Version {version} not recognized")
Expand Down