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

avoid potential circular ref by lazy import #1213

Merged
merged 1 commit into from Oct 21, 2019
Merged

avoid potential circular ref by lazy import #1213

merged 1 commit into from Oct 21, 2019

Conversation

akrherz
Copy link
Contributor

@akrherz akrherz commented Oct 18, 2019

Description Of Changes

Future proof code to allow metpy.calc.basic to import functions in metpy.calc.tools without a potential circular import.

Checklist

@akrherz
Copy link
Contributor Author

akrherz commented Oct 18, 2019

I suppose this could be considered an API break since this will fail after this PR:

from metpy.calc.tools import height_to_pressure_std

shrug.

@zbruick zbruick added this to the 0.12 milestone Oct 18, 2019
@zbruick zbruick added Area: Calc Pertains to calculations Type: Maintenance Updates and clean ups (but not wrong) labels Oct 18, 2019
@lgtm-com
Copy link

lgtm-com bot commented Oct 18, 2019

This pull request fixes 15 alerts when merging 5ea3cd0 into 8c1ca0a - view on LGTM.com

fixed alerts:

  • 15 for Module-level cyclic import

@zbruick
Copy link
Contributor

zbruick commented Oct 18, 2019

Do you mean from tools or from basic? If we can still do from metpy.calc.basic import height_to_pressure_std, then the intended API doesn't break, which I think we should still be able to do.

@akrherz
Copy link
Contributor Author

akrherz commented Oct 18, 2019

Do you mean from tools or from basic?

From tools, since tools is currently doing this global import and thus exposing the basic function there. Do I get bonus points for resolving 15 LGTM issues, lol.

@zbruick
Copy link
Contributor

zbruick commented Oct 18, 2019

Yeah we'll add it to our running list of brownie points. I'll shrug with you on this "API break". If users were importing it this way, they shouldn't have been.

@dopplershift
Copy link
Member

I only consider metpy.calc to be the public API. Submodules in there are subject to rearranging at any time without warning. That’s why we go to the effort we do to put things in the metpy.calc namespace. All the docs, examples, and tests only use metpy.calc. If you see something that’s not, we need to fix it.

@zbruick zbruick merged commit a7c278e into Unidata:master Oct 21, 2019
@dopplershift
Copy link
Member

@akrherz What prompted you to see this? All the complaints from LGTM?

@akrherz
Copy link
Contributor Author

akrherz commented Oct 21, 2019

@akrherz What prompted you to see this? All the complaints from LGTM?

My first paragraph in #1211 was what I was up to. I was attempting to add a dimensionality helper (#1209) in tools.py and realized the circular import possibility. I had not looked at LGTM.

@akrherz akrherz deleted the issue1211 branch October 21, 2019 16:24
@dopplershift
Copy link
Member

UGH. I hadn't even seen that issue. Thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metpy.calc.tools imports metpy.calc.basic which creates potential circular import
3 participants