Skip to content

Conversation

@jrleeman
Copy link
Contributor

Use the matplotlib deprecation framework and deprecate get_upper_air_data. We can/should decide on which version that is official. Closes #516

@dopplershift
Copy link
Member

dopplershift commented Jul 21, 2017

Not wild about adding a top-level metpy.deprecation module. Can we move to cbook?

@jrleeman
Copy link
Contributor Author

We could. It's a decent chunk of code and seemed well encapsulated in its own module. What are the cons of having it separate in this case?

@dopplershift
Copy link
Member

I wasn't wild about having a top-level metpy.deprecation module. I guess there's no harm since we don't have to put it in the API docs.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

There's at least one flake8 failure. Also, it would be good for the top of deprecation.py to have the Matplotlib license.

@jrleeman
Copy link
Contributor Author

Yep, flake8 needs some help, on the list for today hopefully. I'm not dead set on it being a separate module, but seemed like a nice unit of code that made more sense this way than as part of cbook.

@@ -0,0 +1,316 @@
# Copyright (c) 2008-2017 MetPy Developers.
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to blacklist the license check for this file in setup.cfg rather than, essentially, an incorrect block before the proper one. It could then go back to being a comment.



@deprecated('0.8', addendum=' This function is being moved to the Siphon package.',
pending=True)
Copy link
Member

Choose a reason for hiding this comment

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

Why pending? Doesn't pending mean "this will soon be deprecated"? Rather than, this will be deprecated in 0.6 and removed in 0.8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pending Message:

/Users/johnleeman/miniconda3/envs/metpydev/lib/python3.6/site-packages/ipykernel_launcher.py:5: MetpyDeprecationWarning: The get_upper_air_data function will be deprecated in a future version. This function is being moved to the Siphon package.

Without pending:

/Users/johnleeman/miniconda3/envs/metpydev/lib/python3.6/site-packages/ipykernel_launcher.py:5: MetpyDeprecationWarning: The get_upper_air_data function was deprecated in version 0.8. This function is being moved to the Siphon package.

Copy link
Member

Choose a reason for hiding this comment

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

Then I'd say change the version to '0.6' and leave it as not pending since it's not actually pending--it's done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of it as pending since it's still in MetPy, but I see what you're saying. Update coming.

@dopplershift dopplershift added Area: IO Pertains to reading data Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong) labels Jul 21, 2017
@dopplershift dopplershift added this to the 0.6 milestone Jul 21, 2017
@jrleeman
Copy link
Contributor Author

jrleeman commented Aug 4, 2017

Do we want to move on this in 0.6 @dopplershift - with the functionality not being in Siphon yet?

@dopplershift
Copy link
Member

No, the plan was to cut another Siphon release before the MetPy release.

@jrleeman
Copy link
Contributor Author

jrleeman commented Aug 7, 2017

So, yes we want to move on it 😉 Sounds good.

def finalize(wrapper, new_doc):
try:
pass
#obj.__doc = new_doc

Choose a reason for hiding this comment

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

E265 block comment should start with '# '


def finalize(wrapper, new_doc):
wrapper = functools.wraps(func)(wrapper)
#wrapper.__doc__ = new_doc

Choose a reason for hiding this comment

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

E265 block comment should start with '# '


def finalize(wrapper, new_doc):
wrapper = functools.wraps(func)(wrapper)
#wrapper.__doc__ = new_doc

Choose a reason for hiding this comment

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

E265 block comment should start with '# '

@jrleeman
Copy link
Contributor Author

jrleeman commented Aug 7, 2017

I'll squash down once you have a look - wanted to make the diff easier to see.

Copy link
Member

@dopplershift dopplershift 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 now. Just squash down.

@jrleeman jrleeman added Status: Needs Review Pull request needs review Status: Team Discussion Development team needs to discuss issue to decide how to proceed and removed Status: Needs Review Pull request needs review labels Aug 8, 2017
@jrleeman jrleeman removed the Status: Team Discussion Development team needs to discuss issue to decide how to proceed label Aug 23, 2017
@dopplershift
Copy link
Member

I think this is on hold until we have the final resting place for the upper air code in siphon.

@jrleeman
Copy link
Contributor Author

@dopplershift - Should be good to go. I think we're safe ignoring codecov/codacy as the dep tool is straight from matplotlib.

@dopplershift dopplershift merged commit b29d102 into Unidata:master Sep 13, 2017
@jrleeman jrleeman deleted the Upperair_Warning branch September 26, 2017 21:43
@jrleeman jrleeman restored the Upperair_Warning branch September 26, 2017 21:43
@jrleeman jrleeman deleted the Upperair_Warning branch September 26, 2017 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: IO Pertains to reading data Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants