Fix package imports from setup file. #513
Conversation
| @@ -134,11 +141,6 @@ def check_python_version(): | |||
| ) | |||
|
|
|||
| def additional_commands(): | |||
| # Pygments 2 isn't supported on Python 3 versions earlier than 3.3, so | |||
There was a problem hiding this comment.
We already have a python version check (check_python_version) that excludes these versions of Python, so this check is redundant.
Codecov Report
@@ Coverage Diff @@
## master #513 +/- ##
=========================================
Coverage ? 65.25%
=========================================
Files ? 44
Lines ? 7058
Branches ? 1415
=========================================
Hits ? 4606
Misses ? 2027
Partials ? 425
Continue to review full report at Codecov.
|
| @@ -178,7 +180,7 @@ def additional_commands(): | |||
| description='explicitly typed attributes for Python', | |||
| long_description=open('README.rst').read(), | |||
| download_url='https://github.com/enthought/traits', | |||
| install_requires=__requires__, | |||
| install_requires=["six"], | |||
There was a problem hiding this comment.
given this change, do you want to remove the duplication in traits.__init__?
There was a problem hiding this comment.
Possibly. That's an actual package change rather than just an infrastructure change, so it has the potential to break code. It's probably safe to assume that no-one is doing from traits import __requires__, but it's hard to be sure.
| @@ -124,8 +132,7 @@ def check_python_version(): | |||
|
|
|||
| if __name__ == "__main__": | |||
| check_python_version() | |||
There was a problem hiding this comment.
is there a reason why we don't simply use the python_requires kwarg? See https://setuptools.readthedocs.io/en/latest/setuptools.html#new-and-changed-setup-keywords
There was a problem hiding this comment.
None that I know of, but this is unrelated to this PR, which is focused on making the minimal changes necessary to avoid package imports. Please could you open an issue (or a PR)?
| @@ -58,6 +59,10 @@ def _minimal_ext_cmd(cmd): | |||
|
|
|||
|
|
|||
| def write_version_py(filename='traits/_version.py'): | |||
| """ | |||
There was a problem hiding this comment.
Possible nit-picky comment - the function is write_version_py but now it's being used to get the full version info, with the side-effect of also writing the version file.
It might be cleaner and more readable to split this up into a get_version, which uses write_version_py in the absence of a traits/_version.py file.
There was a problem hiding this comment.
Yes, it's a mess that needs to be rewritten. I didn't really want to do that in this PR - this was supposed to be a quick fix for an actual bug. I'll take a look.
There was a problem hiding this comment.
I've just been trying to do a minimal refactor, but there's no such thing: as soon as I try to refactor, things get messy. I'm going to leave this as it is for this PR and delegate reworking the version machinery to a separate PR.
|
Closing this in favour of the more extensive changes in #515. |
Fixes #478.