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

DEP: Give deprecation warning for arguments in kwargs #701

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

tnatt
Copy link
Collaborator

@tnatt tnatt commented Jun 18, 2024

PR to start emitting a deprecation warning for arguments coming in through kwargs in the ExportData.generate_metadata() and ExportData.export() methods.

Addresses #525

@tnatt tnatt marked this pull request as draft June 18, 2024 07:12
@tnatt tnatt force-pushed the deprecate_kwargs branch from 9108c9f to 45458b7 Compare June 18, 2024 07:13
Comment on lines +693 to +710
self._classification = self._get_classification()
self._rep_include = self._get_rep_include()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved into the update_check_settings() as it is only needed to run if any of classification/rep_include/access_ssdl is coming in through kwargs. And hence it can be removed together with the entire update_check_settings() method when the deprecation period has ended.

tests/conftest.py Outdated Show resolved Hide resolved
src/fmu/dataio/dataio.py Outdated Show resolved Hide resolved
@tnatt tnatt force-pushed the deprecate_kwargs branch from 45458b7 to ca9c8f7 Compare June 18, 2024 07:51
@mferrera
Copy link
Collaborator

mferrera commented Jun 18, 2024

I think this is a solid approach.

I am not sure it's worth the effort, but we could consider creating a TypedDict that captures everything we might expect to come through **kwargs and annotate its type in export, generate_metadata, update_check_settings as Unpack[ExportDataOptions] (see PEP 692). This is some upfront effort for something we plan to remove anyway though, and I'm not sure of the value-add except making discussing the available arguments a bit easier.

@tnatt tnatt force-pushed the deprecate_kwargs branch 4 times, most recently from b23df43 to 3bd3866 Compare June 24, 2024 07:36
@tnatt tnatt marked this pull request as ready for review June 24, 2024 08:54
@tnatt tnatt force-pushed the deprecate_kwargs branch from 3bd3866 to 736840a Compare June 24, 2024 09:09
@tnatt tnatt requested a review from jcrivenaes June 24, 2024 10:22
Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

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

Nice work

@tnatt tnatt force-pushed the deprecate_kwargs branch from 736840a to 7e44d69 Compare June 24, 2024 10:56
@tnatt tnatt force-pushed the deprecate_kwargs branch from 7e44d69 to 4433c48 Compare June 24, 2024 10:57
@tnatt tnatt merged commit d072a18 into equinor:main Jun 24, 2024
13 checks passed
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.

3 participants