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

ENH: Add dunder accessors to Pydantic RootModels #767

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

ErichSuter
Copy link
Contributor

@ErichSuter ErichSuter commented Aug 27, 2024

Resolves #687

@ErichSuter ErichSuter self-assigned this Aug 27, 2024
from fmu.dataio._model.global_configuration import Stratigraphy, StratigraphyElement

# --------------------------------------------------------------------------------------
# Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. I guess this heading should be Stratigraphy

# Parameters
# --------------------------------------------------------------------------------------

testset_stratigraphy = Stratigraphy(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could consider implementing this as a fixture in pytest. Then you can take the fixture as input to your test. Example here:

@pytest.fixture(name="direct_creation", scope="function")

More info here https://www.geeksforgeeks.org/fixtures-in-pytest/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, this would be nice to have as a fixture

@slangeveld
Copy link
Contributor

You probably need the prefix "ENH" in the title of the PR as well since the squash merge will use the PR title as the commit message.

@slangeveld
Copy link
Contributor

slangeveld commented Aug 28, 2024

I see is also this class AnyData using the RootModel as base that might need to have the dunders

class AnyData(RootModel):

@slangeveld
Copy link
Contributor

Nice work 👍 Added a few comments. Note the mypy check that is failing.

@mferrera should probably also take a look at this.

@slangeveld slangeveld requested a review from mferrera August 28, 2024 07:04
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

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

This looks good. I think fixing the type annotations and using fixtures will make this good-to-go. There might be some more complaints from mypy; we had this experience with @slangeveld implementing __iter__ on another class if I recall

# Parameters
# --------------------------------------------------------------------------------------

testset_stratigraphy = Stratigraphy(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, this would be nice to have as a fixture

# Parameters
# --------------------------------------------------------------------------------------

testset_params = Parameters(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixtures often go into a conftest.py if they are used in multiple places. There is always one in the root of the test structure, but we can also put them into nested directories. It will check the ones in nested directories first before walking down to the test root.

In this case, since the fixtures are so specific to these tests, it's best to just define them within the same test module file, not in a conftest.py

Comment on lines 137 to 140
def __iter__(self) -> Dict[str, Union[Parameters, int, float, str]]:
return iter(self.root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type annotation is not quite right. iter() creates an iterator over the Dict[str, Union[Parameters, int, float, str]] so it won't return the dict, and by default iterating over the dict iterates over the keys of the dict.

So it should be:

Suggested change
def __iter__(self) -> Dict[str, Union[Parameters, int, float, str]]:
return iter(self.root)
def __iter__(self) -> Iterator[str]:
return iter(self.root)

because we have the keys as strings: Dict[str, ...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was a little bit too quick ...

Comment on lines 111 to 114
def __iter__(self) -> Dict[str, StratigraphyElement]:
return iter(self.root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be the same thing here with returning an Iterator. However, we may need to see if mypy likes this or not 😄

@ErichSuter ErichSuter changed the title Add dunder accessors to Pydantic RootModels ENH: Add dunder accessors to Pydantic RootModels Aug 28, 2024
@ErichSuter
Copy link
Contributor Author

Thanks for your comments, guys :-)
@slangeveld, I also considered the AnyData class. Not obvious at all, but it isn’t an iterable.

@ErichSuter ErichSuter force-pushed the 687-add-dunder-accessors branch from 3aeaeec to ce22180 Compare August 30, 2024 11:48
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

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

🚀

Remember to squash merge or rebase into a single commit before merging

Copy link
Contributor

@slangeveld slangeveld left a comment

Choose a reason for hiding this comment

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

Looks good 😄

@ErichSuter ErichSuter force-pushed the 687-add-dunder-accessors branch from ce22180 to f66fa73 Compare September 2, 2024 10:23
@ErichSuter ErichSuter marked this pull request as ready for review September 2, 2024 10:57
@ErichSuter ErichSuter merged commit c58071a into equinor:main Sep 2, 2024
13 checks passed
@ErichSuter ErichSuter deleted the 687-add-dunder-accessors branch September 2, 2024 11:01
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.

Add dunder accessors to Pydantic RootModels
3 participants