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
Immutable VersionInfo #97
Conversation
I don't understand why CI is broken with pypy and Python 2.7. Any idea? |
CI fixed for Python 2.7 and PyPi |
semver.py
Outdated
def build(self): | ||
return self._build | ||
|
||
def __setattr__(self, name, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought also that setattr wasn't necessary...
but not with Python 2.7 and Pypy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to have private attributes be mutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right ...
This build shows that __setattr__
is not required.
Although what is required for Python 2.7 and Pypi is inheriting VersionInfo from object
.
@@ -414,3 +414,42 @@ def test_parse_method_for_version_info(): | |||
s_version = "1.2.3-alpha.1.2+build.11.e0f985a" | |||
v = VersionInfo.parse(s_version) | |||
assert str(v) == s_version | |||
|
|||
|
|||
def test_immutable(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a test to check if it's possible to add new attribute? Something like this:
try:
v.some_attribute = 1
except:
...
semver.py
Outdated
def build(self): | ||
return self._build | ||
|
||
def __setattr__(self, name, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to have private attributes be mutable?
semver.py
Outdated
|
||
def __setattr__(self, name, value): | ||
if name not in self.__slots__: | ||
raise AttributeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if so, it's better to pass name to AttributeError to avoid any confusions.
Closes #96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, thanks :)
Closes #96