Skip to content

Conversation

@wjbenfold
Copy link
Contributor

@wjbenfold wjbenfold commented Jan 20, 2022

🚀 Pull Request

Description

See #4394

To do:

  • Work out whether this is the right way to deprecate a module - the warning for abf at least currently shows up on import of fileformats, which we may want to avoid. Not sure where to put it instead though - in every function?
  • Check wording of message and consider mentioning future mitigations (undo deprecation, use plugin architecture?)
  • Fix doctest that's failing due to the deprecation warnings

Consult Iris pull request check list

@wjbenfold wjbenfold mentioned this pull request Jan 20, 2022
8 tasks
@wjbenfold wjbenfold self-assigned this Jan 20, 2022
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Looking good, but I agree we need to avoid the message when "just" importing iris.fileformats.

As with the other deprecations, I'd also like to be sure @lbdreyer had a chance to OK this.

That warning on any use of iris.fileformats happens because Iris imports abf to populate its file format handler structures.
I think it still makes sense to issue the warning whenever abf is imported, so I think the
nice way is to use a deferred import within a wrapper function.
I think I will propose something similar as a PR to your branch...

@pp-mo
Copy link
Member

pp-mo commented Jan 21, 2022

I think I will propose something

See wjbenfold#2

@wjbenfold
Copy link
Contributor Author

I think I will propose something

See wjbenfold#2

I love the elegance of this solution

@lbdreyer
Copy link
Member

As with the other deprecations, I'd also like to be sure @lbdreyer had a chance to OK this.

I'm fine with this. If we are happy to reverse the decision (or perhaps come up with an alternative!) should someone say they are still using it then that is fine.

@wjbenfold wjbenfold marked this pull request as ready for review January 24, 2022 15:41
@wjbenfold wjbenfold force-pushed the wjbenfold-deprecate-file-formats branch from 88b065a to 0942757 Compare January 24, 2022 17:09
@pp-mo
Copy link
Member

pp-mo commented Jan 25, 2022

hi @wjbenfold
something weird has happened here -- although you merged my suggestion wjbenfold#2 there is no change to iris.fileformats.__init__.py in the PR changed files
So I think something was wrong with the merge operation + you need to address it so we get what we need here

I think that's also why you are getting the docs-build fail :
We have a shim in all our code examples (e.g. here) that promotes warnings to errors,
just so we can ensure that our examples don't trigger any warnings

So the examples hit a deprecation warning when importing iris.fileformats, and that --> error in the docs build

@wjbenfold
Copy link
Contributor Author

Ah, didn't pull before the rebase, will do that

@wjbenfold wjbenfold force-pushed the wjbenfold-deprecate-file-formats branch from 0942757 to 3389037 Compare January 25, 2022 12:30
@wjbenfold
Copy link
Contributor Author

I believe that this is ready now @pp-mo

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Ok now!

@pp-mo pp-mo merged commit 851c12d into SciTools:main Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants