Future: Python 3.8 compatibility

[michaelaye]

This warning will need to be addressed in the future, as it would break in Python 3.8:

spiceypy/tests/test_spiceerrors.py::test_getSpiceyException
  /Users/klay6683/Dropbox/src/SpiceyPy/spiceypy/utils/support_types.py:107: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
    return isinstance(i, collections.Iterable) and not isinstance(i, six.string_types)

[jessemapel]

Looks like six was going to support this natively, but has decided not to for some reason.

benjaminp/six#241

Other options appear to be:

if six.PY3:
  from collections.abc import Iterable
else:
  from collections import Iterable

and

try:
    import collections.abc as collections_abc
except ImportError:
    import collections as collections_abc

I'm inclined towards leveraging six if possible.

[michaelaye]

Can you elaborate why you’d prefer leveraging six if, in this rare case, we actually could have a back- and forward compatible solution that never needs to change even if we were to get rid of six in the future? Somehow it sound advantageous to me to stay with built-in Python only as much as possible?

[jessemapel]

I much prefer testing on the python version instead of just catching an importError. try/catch style tests tend to cause headaches when something unexpected raises an exception.

[AndrewAnnex]

The very point of six are to perform these python version specific delegations so I do not want to maintain my own version of this, although there may not be an option to do so because of six. In reading the post it look like the maintainer considers 3.8 related changes unnecessary given Python 2.7 EOL. I disagree with that but it could add another item to my list of reasons to drop python 2 support...

…

-Andrew Annex

On Nov 22, 2019, at 6:36 PM, Jesse Mapel ***@***.***> wrote:  I much prefer testing on the python version instead of just catching an importError. try/catch style tests tend to cause headaches when something unexpected raises an exception. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[AndrewAnnex]

six is also a very popular and small dependency, I don't plan on ever removing it from the package unless forced

[michaelaye]

@jessemapel can you give an example how exception handling causes headaches for import checking? I don't understand what can be problematic here, either an import works or not. And for checking on python version, you really don't need six to do that, sys.version_info.major is built-in.

@AndrewAnnex I have nothing against six, it was and is a very useful and popular package. But why use it when it doesn't provide any benefit? And as you wrote above, thinking about dropping Py2 support, that would be a nice time to remove all use of six, as it reduces the codebase and therefore the maintenance burden. scikit-image just went through the same effort, after dropping legacy python support.

[AndrewAnnex]

it provides benefit for supporting python 2. If I drop python 2 support then there would not be a reason to use it.

however, a lot of my users use python 2, and they do not like being told to upgrade, so I am not actively planning to drop python 2 support any time soon.

[michaelaye]

you misunderstand. my question was why use six at a specific point in the code when there's no benefit for that code and built-in solutions work just as well? The less specific six uses you have in your code, the easier it will be to clean up the code base once you are dropping Py2 support and future you will love past you for that. ;)

[AndrewAnnex]

I understood your question perfectly well, but this isn't an urgent issue for me, and I disagree with many of the benefits you cite.

[michaelaye]

If you understood my question well I’m puzzled why instead of addressing my point you chose to inform me that the benefit of six is to support Python 2?
I also will note that disagreement without a single argument brought forward isn’t really productive and as you don’t seem to want to engage in a productive discussion, I will be out of your hair. Sorry if I caused any inconvenience.

[jessemapel]

@jessemapel can you give an example how exception handling causes headaches for import checking? I don't understand what can be problematic here, either an import works or not. And for checking on python version, you really don't need six to do that, sys.version_info.major is built-in.

Using sys.version_info.major seems like the best way to handle this then 👍.

With the try catch logic, if a user is using Python 2 and has somehow mangled their environment, they won't get the real error message from the Python 2 import call. They will get a package not found error from the Python 3 import call. To get to the real error message, you then have to go in and comment out the try catch logic. Since this is a standard package it would take some work to get your version of Python to this state, but I don't see any draw backs to checking the version number. Plus the try catch logic will still raise a warning for Python <3.8.

[jessemapel]

Also, what's the cost/benefit of removing/retaining Python 2 support. It seems like keeping it in is a fairly low cost with a high benefit. Taking it out is a fairly high cost with low benefit.

[michaelaye]

Having to read a code base is an always underestimated cost. Have a look here how much “trash” can be removed when dropping Python 2 support. It’s quite a thinning. https://adamj.eu/tech/2019/03/13/dropping-python-2-support/

If the exception handling would always create a warning < 3.8, that’s a good reason not to use it. But why would it do that? Should try it.

[jessemapel]

If the exception handling would always create a warning < 3.8, that’s a good reason not to use it. But why would it do that? Should try it.

With Python < 3.8, import collections.abc as collections_abc won't raise an exception so it will still successfully execute and raise the warning.

[michaelaye]

sorry, what warning? as that import is fine in 3.7, nothing happens at those lines, i must misunderstand you. do you speak of another warning?

[michaelaye]

Oh, you mean the warning of the tests for this issue?

[michaelaye]

So, still confused what you mean with still having a warning < 3.8?

I just tried this in 3.7:

try:
    import collections.abc as collections_abc
except ImportError:
    import collections as collections_abc

and there's no warning, as expected.

[jessemapel]

@michaelaye I had mixed up if it does the Python 3 or Python 2 first. The warning shouldn't occur because the Python 3 approach succeeds.