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

WIP: Add API tests for metadata handling #107

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

oiffrig
Copy link

@oiffrig oiffrig commented Jun 6, 2023

A concrete example on how we could handle metadata. Still work in progress and open for discussion.

@FussyDuck
Copy link

FussyDuck commented Jun 6, 2023

CLA assistant check
All committers have signed the CLA.

f = from_source("file", earthkit_examples_file("test_single.grib"))
md = f[0].metadata()

assert isinstance(md, GribMetadata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you suggest only to change the behaviour of GribField.metadata() when called without arguments, so, the rest of the functionality would be kept?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that was the idea. We keep the current API, but change the underlying metadata object, and we return it if the user is not asking for specific keys/namespaces

assert md.get("shortName") == "2t"
with pytest.raises(KeyError):
md["nonExistentKey"]
assert md.get("nonExistentKey", 12) == 12
Copy link
Collaborator

Choose a reason for hiding this comment

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

What should happen for:

md.get("nonExistentKey")

Should we raise KeyError or return None as a dict would do?

Copy link
Author

Choose a reason for hiding this comment

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

I would return None, to match dict and the behaviour of the default value when given

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @oiffrig! There is another question related to the type cast. On a Field we can call metadata() like this to specify the value type we want:

f.metadata("centre", astype=str)
f.metadata("centre", astype=int)
f.metadata("centre:s")
f.metadata("centre:str")
f.metadata("centre:l")
f.metadata("centre:int")

How would the Metadata object support these? The last 4 calls are of course supported for GRIB by [] and get().

Copy link
Author

Choose a reason for hiding this comment

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

Not totally sure. What I would suggest is to have that as an overridable behaviour: the metadata object knows a few types and can convert from them (raising an error if the type is not supported), and specific implementations (e.g. GRIB) can extend the default behaviour by using functionalities of the underlying backend

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Would that mean that get should work like that:

md.get("centre", astype=int)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants