Drop support for IPython < 3.0. #237

Merged
merged 1 commit into from Jun 16, 2015

Conversation

Projects
None yet
2 participants
@moorepants
Member

moorepants commented Jun 16, 2015

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.

  • There are no merge conflicts.
  • If there is a related issue, a reference to that issue is in the
    commit message.
  • Unit tests have been added for the bug. (Please reference the issue #
    in the unit test.)
  • The tests pass both locally (run nosetests) and on Travis CI.
  • The code follows PEP8 guidelines. (use a linter, e.g.
    pylint, to check your code)
  • The bug fix is documented in the Release
    Notes
    .
  • The code is backwards compatible. (All public methods/classes must
    follow deprecation cycles.)
  • All reviewer comments have been addressed.

@moorepants moorepants added this to the 0.3.0 Release milestone Jun 16, 2015

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 16, 2015

Member

@pydy/pydy-developers This needs a review for the 0.3.0 release.

Member

moorepants commented Jun 16, 2015

@pydy/pydy-developers This needs a review for the 0.3.0 release.

@oliverlee oliverlee changed the title from Drop support for IPython <= 3.0. to Drop support for IPython < 3.0. Jun 16, 2015

pydy/viz/scene.py
+ msg = ('PyDy only supports IPython >= 3.0.0. You have '
+ 'IPython {} installed. IPython related functionalities will '
+ 'not be available')
+ with warnings.catch_warnings():

This comment has been minimized.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

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.

@moorepants

moorepants Jun 16, 2015

Member

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.

This comment has been minimized.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

So what is the difference in Python 3 about this. I don't understand.

@moorepants

moorepants Jun 16, 2015

Member

So what is the difference in Python 3 about this. I don't understand.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

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.

@moorepants

moorepants Jun 16, 2015

Member

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.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

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.

@moorepants

moorepants Jun 16, 2015

Member

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.

This comment has been minimized.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

@oliverlee

oliverlee Jun 16, 2015

Contributor

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.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

Thanks, I'll change to what I said above.

@moorepants

moorepants Jun 16, 2015

Member

Thanks, I'll change to what I said above.

@oliverlee

This comment has been minimized.

Show comment
Hide comment
@oliverlee

oliverlee Jun 16, 2015

Contributor

Please change the commit title to:
Drop support for IPython < 3.0

Contributor

oliverlee commented Jun 16, 2015

Please change the commit title to:
Drop support for IPython < 3.0

Drop support for IPython < 3.0.
Fixes #235.

- Adds in a PyDyImportWarning.
- Also add PR links to some items in the release notes.
- Fixes an old error in MANIFEST.in.
@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 16, 2015

Member

Ok, things are fixed.

Member

moorepants commented Jun 16, 2015

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`_]

This comment has been minimized.

@oliverlee

oliverlee Jun 16, 2015

Contributor

Why is this here? I thought it was already merged with Master.

@oliverlee

oliverlee Jun 16, 2015

Contributor

Why is this here? I thought it was already merged with Master.

This comment has been minimized.

@moorepants

moorepants Jun 16, 2015

Member

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.

@moorepants

moorepants Jun 16, 2015

Member

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.

This comment has been minimized.

@oliverlee

oliverlee Jun 16, 2015

Contributor

cool thanks

@oliverlee

oliverlee Jun 16, 2015

Contributor

cool thanks

oliverlee added a commit that referenced this pull request Jun 16, 2015

@oliverlee oliverlee merged commit 4f342a0 into pydy:master Jun 16, 2015

1 check passed

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

@moorepants moorepants deleted the moorepants:drop-ipython2 branch Jun 18, 2015

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