-
Notifications
You must be signed in to change notification settings - Fork 96
VersionInfo.parse should be a class method #276
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
Comments
Thanks @Anvil for your issue and the pull request. 👍 That's an interesting approach. I see your point. However, not entirely sure if changing Apart from that, the Do you need it for Python2 only or do you use it for Python3? Thanks for your pull request (PR). As the above discussion is not really solved, I would like to wait for this issue until the above issue is remedied. Just a short comment about your PR: you don't have to change anything right now (due to the open discussion). It's just a reminder for future PRs. We require test cases for each new feature. 😉 If a feature changes the public API, this also needs to be documented. Ideally with some code examples. For further details, refer to our Contributing to semver section. |
Hello As far as I can tell, there wont be any breaking by changing static to class. The reference "VersionInfo.parse" makes it automatically bound to the class. In general, all factory methods (methods that return an instance of the class) should be class methods, not staticmethods. I dont use python 2 anymore. Do you intent do keep compatibility with python2 ? |
I can provide tests and examples if it's accepted, though. Feel free to ask. |
A basic yet (i think) meaningful test case / example, would be to have a subclass that accepts "v1.2.3" as semver, so that you can directly feed git tag ot the parse method. The subclass would have a parse such as: class GitTagSemVer(VersionInfo):
@classmethod
def parse(cls, version: str):
if not version.startswith('v'):
raise ValueError(f"{version}: not a valid semantic version tag")
return super().parse(version[1:])
def __str__(self):
# Reconstruct the tag.
return "v" + super().__str__()
import git
import logging
def semver_tags(path):
repo = git.Repo(path)
for tag in repo.tags:
try:
yield GitTagSemVer.parse(tag)
except ValueError:
logging.error(f"{tag}: bad semver tag") |
Thank you @Anvil for all the ideas. Sorry for not responding to it earlier. I hope I can work on that soon. Your ideas and arguments make sense to me. 👍
That's a good question. If possible, I would love to have at least this issue compatible between the two versions. It doesn't look like a big problem. But let me think of it again, once I'm coming back to this issue. |
I'm glade it makes sense. On my part, there is no requirement nor need for a python2 compatibility. My team and I are working with python >= 3.6. Thank you for your work ! |
Absolutely! 👍 I'm glad you've reached out to us and contributed.
That's great! I've added a small test case; there I had to adapt the Plan is to release 2.11.0 soon and start working on Python 3. Otherwise these compatibility things will always bite me. 😆 |
Integrated in commit 6e4b402 so closing this issue. |
Related to issue python-semver#276
Related to issue python-semver#276
Related to issue python-semver#276
* python-semver#274 / python-semver#275 String Types Py2 vs. Py3 compatibility * python-semver#277 Turn VersionInfo.parse into classmethod to allow subclasses * python-semver#286 Add author and update changelog for python-semver#276/python-semver#277
Related to issue python-semver#276
Related to issue python-semver#276
Uh oh!
There was an error while loading. Please reload this page.
I wanted to create a subclass of VersionInfo for company reasons (more checkings, more methods, etc) and it turned out the VersionInfo.parse is a staticmethod, always returning VersionInfo instances, no matter what.
By turning the
parse
static method into a class method and making itreturn cls(**version_parts)
the result of parse will be an instance of the subclass (my class), not the superclass (VersionInfo).e.g:
Current result is :
<class 'semver.VersionInfo'>
, while I would have expected<class 'MyBogusVersionInfo'>
The text was updated successfully, but these errors were encountered: