Skip to content

Comments

Remove namedtuple inheritance#93

Merged
s-celles merged 1 commit intopython-semver:masterfrom
s-celles:no_named_tuple
May 26, 2018
Merged

Remove namedtuple inheritance#93
s-celles merged 1 commit intopython-semver:masterfrom
s-celles:no_named_tuple

Conversation

@s-celles
Copy link
Member

@s-celles s-celles commented May 26, 2018 •
edited
Loading

Closes #87
Closes #94

@s-celles
Remove namedtuple inheritance
0e1c130 
Closes #87
Closes #94
Add entries to CHANGELOG
@s-celles s-celles merged commit 2754b7e into python-semver:master May 26, 2018
kxepal
kxepal reviewed May 29, 2018
View reviewed changes
semver.py
:param str prerelease: an optional prerelease string
:param str build: an optional build string
"""
__slots__ = ()
Copy link
Contributor

@kxepal kxepal May 29, 2018

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

@s-celles s-celles May 29, 2018 •
edited
Loading

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

@ppkt ppkt May 29, 2018

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

@s-celles s-celles May 29, 2018

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

@s-celles s-celles mentioned this pull request May 29, 2018
Closed
@hugobuddel hugobuddel mentioned this pull request Oct 10, 2018
Open
@hugobuddel
Copy link

hugobuddel commented Oct 10, 2018

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]

@ppkt ppkt mentioned this pull request Oct 10, 2018
Closed
hugobuddel added a commit to hugobuddel/releash that referenced this pull request Oct 10, 2018
@hugobuddel
Fix problem with semver 2.8.1
134dd88 
VersionInfo from semver 2.8.1 is not iterable, see python-semver/python-semver#93
@hugobuddel
Copy link

hugobuddel commented Oct 10, 2018

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

Reviewers

@ppkt ppkt ppkt left review comments

+1 more reviewer

@kxepal kxepal kxepal left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@s-celles @hugobuddel @ppkt @kxepal