-
Notifications
You must be signed in to change notification settings - Fork 112
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
Drop support for IPython < 3.0. #237
Conversation
@pydy/pydy-developers This needs a review for the 0.3.0 release. |
msg = ('PyDy only supports IPython >= 3.0.0. You have ' | ||
'IPython {} installed. IPython related functionalities will ' | ||
'not be available') | ||
with warnings.catch_warnings(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use catch_warnings()
. It should be used if you expect someone else's code to throw warnings.
Also why do you need to specify once
? This module shouldn't be imported multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default Python does not show warnings and I only want these warnings to show once per python session. So if you happen to import this multiple times in single session it will only issue the import warning once.
catch_warnings
is a nice way to set the filters on a per warning level. I'd also be fine with setting the warning filter to once
globaly for PyDy. If you have a suggestion of a better pattern for per warning filter control, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use the warning filter with once
. The way you have it written was causing the Deprecation Warning to be displayed every single time the method was called when using Python 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what is the difference in Python 3 about this. I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I used this way of setting the filter was because if you set the warnings filter globally in a module, you'll turn on all warnings for that python session, including, I believe, all the warnings that are normally hidden in any external module you import. Maybe this was a problem when we were testing against NumPy 1.6 and SciPy 0.9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You suggestion requires creating sublcassed warnings. I see why you did that in your other PR. I guess I never did that because it seems silly to create a subclass that doesn't have anything overridden. I'll create a PyDyImportWarning
that subclasses ImportWarning
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Python 3, the filter is only applied in the scope of the with warnings.catch_warnings()
block.
Here's a short example:
import warnings
def func():
with warnings.catch_warnings():
warnings.filterwarnings('once'):
warnings.warn('asd', ImportWarning)
warnings.warn('asd', ImportWarning)
for i in range(2):
func()
If you run with Python 2 and Python 3, it prints the warning a different number of times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll change to what I said above.
Please change the commit title to: |
Fixes pydy#235. - Adds in a PyDyImportWarning. - Also add PR links to some items in the release notes. - Fixes an old error in MANIFEST.in.
1cb3616
to
db3b606
Compare
Ok, things are fixed. |
- Added support Python 3.3 and 3.4. [PR `#38`_] | ||
- Bumped up the minimum dependencies for NumPy, SciPy, and Cython. | ||
- Dropped support for IPython < 3.0. [PR `#237`_] | ||
- Added support Python 3.3 and 3.4. [PR `#229`_] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here? I thought it was already merged with Master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased and then changed it here to point to the PR instead of the original issue to follow the same pattern as the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool thanks
Drop support for IPython < 3.0.
Fixes #235.
Also add PR links to some items in the release notes.
I've tested each of the import errors and warnings with no IPython, IPython < 3.0, and IPython 3.0+. Everything works for me locally.
commit message.
in the unit test.)
nosetests
) and on Travis CI.pylint, to check your code)
Notes.
follow deprecation cycles.)