Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove namedtuple inheritance #93

Merged
merged 1 commit into from May 26, 2018
Merged

Remove namedtuple inheritance #93

merged 1 commit into from May 26, 2018

Conversation

scls19fr
Copy link
Member

@scls19fr scls19fr commented May 26, 2018

Closes #87
Closes #94

Closes #87
Closes #94
Add entries to CHANGELOG
@scls19fr scls19fr merged commit 2754b7e into python-semver:master May 26, 2018
@@ -83,7 +82,26 @@ class VersionInfo(collections.namedtuple(
:param str prerelease: an optional prerelease string
:param str build: an optional build string
"""
__slots__ = ()
Copy link
Contributor

Choose a reason for hiding this comment

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

So now VersionInfo is a mutable object with dynamic attrs. That's quite too dramatic change in the name of pretty printting pandas dataframes /:

Copy link
Member Author

@scls19fr scls19fr May 29, 2018

Choose a reason for hiding this comment

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

PR to make VersionInfo immutable (without inheriting from namedtuple) is highly appreciated

Copy link
Member

Choose a reason for hiding this comment

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

But what's wrong with "namedtuple" approach? (Apart from problems with printing in Pandas)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing else than Pandas printing but that's quite problematic as it won't be solved soon (my 2 cts)

Some tests to ensure that VersionInfo are immutable should be added if mutability of VersionInfo is a problem

@hugobuddel
Copy link

It seems this change broke backwards compatibility, because VersionInfo is not iterable anymore.

See maartenbreddels/releash#2 or https://travis-ci.org/maartenbreddels/releash/jobs/439551331 , which worked 7 months ago with semver 2.7.9, but fails now with 2.8.1.

Ironic this happened in the semver package. Should this have been merged into 3.0.0, and not in 2.x.y?

@ppkt
Copy link
Member

ppkt commented Oct 10, 2018

@hugobuddel agree, but we didn't have a test case for this scenario. I'll look on this issue. As workaround you may use:
self.version = [k for k in semver.parse_version_info(new)._astuple() if k is not None]

hugobuddel added a commit to hugobuddel/releash that referenced this pull request Oct 10, 2018
VersionInfo from semver 2.8.1 is not iterable, see python-semver/python-semver#93
@hugobuddel
Copy link

Thanks @ppkt! It seems okay to have this change in a minor version increase because it was supposed to be backwards compatible. The old behavior could be considered undocumented and/or the new behavior can be seen as a bug and be fixed on a patch level (which you just did).

Thanks for fixing it so quickly. I've just written out the list, so that code works with all versions of semver.

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.

None yet

4 participants