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

Marked export to gs as no longer supported #1970

Merged
merged 4 commits into from May 16, 2016

Conversation

Projects
None yet
2 participants
@aashish24
Contributor

aashish24 commented May 13, 2016

No description provided.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 13, 2016

Fixes #1955

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 13, 2016

@doutriaux1 is this is the right way to go about removing a method?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 13, 2016

you might as well remove the doc string all together. Do you want to leave a warning? Or simply remove the function and let the user get an error message?

If you go for warning, please do not send a simple warning, use a "deprecation" warning so we can eventually take it out completely. see: https://docs.python.org/2/library/exceptions.html#exceptions.DeprecationWarning

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 13, 2016

Thanks for the feedback @doutriaux1. I am thinking warning for now as we didn't announce it but we can do that in 2.6 and take it out (Just trying to set a convention here). Can take out the docstring, no longer needed. I used DepreactionWarning but in Python 2.* its ignored by default (https://docs.python.org/2/library/warnings.html). So When nothing happens when I tried it.

Update Canvas.py
Allow for deprecation warnings
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 16, 2016

@aashish24 I tweaked your changes

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 16, 2016

Thansk @doutriaux1 but I guess you didn't see my comment. DeprecationWarning does not result in warning (its ignored by default in 2.7).

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 16, 2016

@aashish24 I guess you didn't see my edits 😉 I added a line to make sure it's not ignored.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 16, 2016

Cool! thanks @doutriaux1 Just a nit pick (only styling thing). I wasn't sure what would the policy we have in place but this works for me.

can this be moved to close to the warning itself?
warnings.filterwarnings("default","",DeprecationWarning,"",0)

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 16, 2016

@doutriaux1 filterwarnings does not work for me. It is possible that I am doing something wrong. Try this example:


In [10]: warnings.filterwarnings("default", "", DeprecationWarning, "", 0)

In [11]: def bar():                                                       
    warnings.warn("foo", DeprecationWarning)
   ....:     

In [12]: bar()

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 16, 2016

If you want to, you can move it closer, I'm ok with it. But I'm thinking we're going to have a few more of these deprecated warnings so I prefer to leave at the top, right after the warnings import.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 16, 2016

I think we just need this:

warnings.simplefilter('default')

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 16, 2016

@doutriaux1 please have a look at my recent change.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 16, 2016

@aashish24 interesting it works for me:

>>> import warnings
>>> warnings.filterwarnings("default", "", DeprecationWarning, "", 0)
>>> def bar():
...   warnings.warn("foo", DeprecationWarning)
... 
>>> bar()
__main__:2: DeprecationWarning: foo
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 16, 2016

@aashish24 yours works for me as well, merging.

@doutriaux1 doutriaux1 merged commit bbfdbd5 into master May 16, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@doutriaux1 doutriaux1 deleted the fix_deprecate_gs branch May 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment