NotificationsYou must be signed in to change notification settings
Fork441
Star1.8k

quick and dirty parser for version number in legacy-defaults#435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

murrayrm merged 2 commits intopython-control:masterfromsawyerbfuller:generalize-legacy-defaults

Oct 17, 2020

Merged

quick and dirty parser for version number in legacy-defaults#435

murrayrm merged 2 commits intopython-control:masterfromsawyerbfuller:generalize-legacy-defaults

Oct 17, 2020

Conversation

Copy link

Contributor

sawyerbfuller commentedJul 23, 2020

follows issue#432

quick and dirty parser for version number in legacy-defaults

77a38fc

Copy link

coveralls commentedJul 23, 2020•
edited
Loading

Coverage increased (+0.03%) to 84.246% when pulling94c57dd on sawyerbfuller:generalize-legacy-defaults intod3142ff on python-control:master.

Copy link

coveralls commentedJul 23, 2020

Coverage increased (+0.03%) to 84.242% when pulling77a38fc on sawyerbfuller:generalize-legacy-defaults intod3142ff on python-control:master.

bnavigator reviewed

Jul 23, 2020

View reviewed changes

control/config.py Outdated

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'spackaging.version.parse?

Copy link

ContributorAuthor

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

bnavigatorJul 24, 2020•
edited
Loading

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 libdistutils.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. 😉)