Skip to content

Conversation

sawyerbfuller
Copy link
Contributor

follows issue #432

@coveralls
Copy link

coveralls commented Jul 23, 2020

Coverage Status

Coverage increased (+0.03%) to 84.246% when pulling 94c57dd on sawyerbfuller:generalize-legacy-defaults into d3142ff on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 84.242% when pulling 77a38fc on sawyerbfuller:generalize-legacy-defaults into d3142ff on python-control:master.

Comment on lines 171 to 173
first_digit = int(version[0])
second_digit = int(version[2])
if second_digit < 8:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will break with >=0.10.0

Maybe use setuptool's packaging.version.parse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found I could make a quick change that addresses this concern - does what I came up with look ok? alternatively, are you sure a dependency on the module you proposed ok to have? (is it always included in python?)

Copy link
Contributor

@bnavigator bnavigator Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an edge case. setuptools should not really be needed after install, or if you get the package from something else. The other option mentioned in that SO question is undocumented but standard lib distutils.version. python-control already uses that within the test suite.

Although your quick solution works, too. I would be fine with it. (Not to imply that I have any authority. I am only here to contribute unwarranted suggestions. 😉)

@bnavigator bnavigator added this to the 0.8.4 milestone Aug 17, 2020
@sawyerbfuller sawyerbfuller linked an issue Aug 19, 2020 that may be closed by this pull request
@murrayrm murrayrm merged commit 7a62428 into python-control:master Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpretation of argument for use_legacy_defaults is not clear
4 participants