Conversation
setup.cfg
Outdated
| coverage | ||
| skyfield>=1.20 | ||
| sgp4>=2.3 | ||
| packaging |
There was a problem hiding this comment.
Shouldn't packaging be a runtime requirement now, like numpy and pyerfa?
There was a problem hiding this comment.
I didn't think so because it is only imported in the tests. Or at least that was the goal -- using packaging in the tests but not in the main package to avoid adding a new dependency.
There was a problem hiding this comment.
Aha, I hadn't realized this would be just for testing - in that case we actually do not need to add a requirement at all, since pytest itself already requires packaging! (It must do its version tests in, e.g., pytest.importorskip with it...).
mhvk
left a comment
There was a problem hiding this comment.
Always good to see actual code, since the worry about a dependency that I had is not warranted. Nice!
For introspection, I'm fine with going the easy route and simply move the import inside, with a comment on why. Alternatively, also fine with just seeing whether a simple use of the output of the regexp that is already there isn't good enough...
I think the regex works for getting the version number -- I don't want to write the code to do the comparison. For now I've just moved the distutils import inside the function in I had not realized that |
It shouldn't be though. Requiring it at runtime is a bug. |
In that case the current fix should do the trick. |
Description
This addresses part of #10578 by removing
distutils.version.LooseVersion.packagingis added as a dependency andpackaging.version.Versionis used instead ofLooseVersion. It does not seem problematic to me to add this as a test dependency.LooseVersion; alternatively, we could just move the import ofLooseVersioninside of the function, which should remove the setuptools warnings.