@tomschr it's a quite interesting idea. I like it :)
In my opinion both solutions looks good but personally I prefer the first one - but I wonder if it's possible to use single dispatch here.

EDIT:
Unfortunately, it seems that it's a new feature in Python 3.4+ (it could be installed via pip but it's an external dependency):
https://www.python.org/dev/peps/pep-0443/
https://pypi.org/project/singledispatch/

@ppkt

it's a quite interesting idea. I like it :)

Thank you. :)

I prefer the first one - but I wonder if it's possible to use single dispatch here.

Ahh, right, you probably mean the one from functool.singledispatch, right?

Yes, that could also be a solution. Not sure if testing would make it a bit more complicated (which wouldn't be an argument against it).

Unfortunately, it seems that it's a new feature in Python 3.4+

Yes, you're right. I found it too when I scrolled down a bit. Argh.

I've looked into the implementation and it doesn't look trivial.

@tomschr Sorry, I wasn't clear - I was talking about your "Suggested solution" - i.e. adding *other argument :)

I also think that it would be great to have an ability to provide a iterable instead of list of versions and I thought that it could be possible with feature called "singledispatch" but, unfortunately, as long as we support Python 2.7 it's not possible without an external package

@ppkt No problem. :-)

it could be possible with feature called "singledispatch" but, unfortunately, as long as we support Python 2.7 it's not possible without an external package

Yes, that's unfortunate.

Theoretically, if we don't want to rely on an external package, we could copy the missing feature and apply it for Python 2.7 only. I know, it would be an ugly solution and only for such a small function would be probably "mad". 😆

By the way, do we have any plans when we are "sunsetting" the support for Python 2.7? Next year, when it's officially not supported anymore?

I know, it's a different issue, but I haven't found any information about how we want to deal with that. Maybe we should open a new issue and document that somewhere (in the README, our documentation etc.)?

@tomschr I would prefer to avoid including external library in our code ;)

Regarding sunsetting Py2.7 - I have no idea. Since semver is quite simple library I think we should keep support for older Python versions as long as it's possible but we should definitely think about moving forward. Maybe releasing version 3.x with dropped support for older versions and refactored to use modern features used in language but keep support for bugfixes in 2.x branch?

@ppkt

@tomschr I would prefer to avoid including external library in our code ;)

Mee too! Just playing devils advocate here. 😉

Regarding sunsetting Py2.7 - I have no idea. [...]

I've opened issue #161 for this to clarify. Funny, we had the same idea. 😉 👍

@ppkt By the way, I maybe found a workaround for this "singledispatch". As we can't use functools.singledispatch (not available for Python 2.7), we could circumvent it. 😉

The pkgutil.simplegeneric is (as the name implies) a simple generic "singledispatch". It is available for Python 2.7 and beyond. Strangely, it's not documented (neither for 2.7, nor for 3.x).

Here is a short demonstration:

from __future__ import print_function
from pkgutil import simplegeneric


@simplegeneric
def max_ver(v1, v2):
    print("> Comparing v1=%d and v2=%d" % (v1, v2))
    return max(v1, v2)

@max_ver.register(list)
@max_ver.register(tuple)
def _(iterable):
    print("> iterable:", iterable)
    return max(iterable)

print(max_ver(2, 5))
print(max_ver((3, 5, 10, 2, 1, 0)))

# Output is:
# > Comparing v1=2 and v2=5
# 5
# > iterable: (3, 5, 10, 2, 1, 0)
# 10

Would that be an option?

Once we remove support for Python 2.7, we can change it to functools.singledispatch if needed.

@tomschr neat :) It seems that it's some kind of backport of singledispatch:
https://github.com/python/cpython/blob/2.7/Lib/pkgutil.py#L28
https://github.com/python/cpython/blob/3.4/Lib/pkgutil.py#L3

After reading the issue again, I believe we should (if at all) implement the changes in 3.0.0. That makes it easier for us and we can use the functools.singledispatch function (if we want to use it).

Could we mark this feature for 3.0.0? Maybe with a GH milestone?

What about tagging?

I must admit I have never use GH milestone previously. What are the pros and cons compared to tagging issues?

@scls19fr

I must admit I have never use GH milestone previously. What are the pros and cons compared to tagging issues?

Good question! 👍 After thinking about again, we have three options:

• Labels
  are a lightweight way to associate an issue with a name. You can filter issues.
• Milestones
  It's another way to structure and associate your issue. An issue now "belongs" to a milestone. If you assign your issues to a specific release, it shows you a percentage value about to what degree the release is ready. Milestones can be closed and reopened like issues.
• Projects
  It's a bit like a Trello board (if you know that Web site) and a way for project managers to get an overview. You can sort your issues into, for example, TODOs (open issues), Doing (issues that are being worked on), and Done (merged/closed issues) columns. You can have more than one board (for example, a board for a 2.9.x release, another board for the 3.x.y release).

I've sorted them from easy to more advanced. Labels are the easiest way, projects the most advanced, and milestones are in the middle.
All options are a good method to structure your issues. I guess, it's also a matter of taste.

You can continue to use labels (as you already did that). If we find out this is not enough, we can try milestones. 😉

Have you tried just using the builtin max() function? It works quite well. I believe this is possible due to the implementation of __lt__ and __gt__.

>>> versions = [VersionInfo(0, 1, 0), VersionInfo(0, 2, 0), VersionInfo(0, 1, 3)]
>>> max(versions)
VersionInfo(major=0, minor=2, patch=0, prerelease=None, build=None)

Ahh, thanks for the hint, Thomas! 👍 Even better! I haven't looked at this issue for some time.

You are right, with the help of the __lt__ and __gt__ operators, it makes comparisons easier. I guess, there is no need for the max_ver and min_ver functions anymore.

Actually, I was about to open a PR to deprecate these functions, but as I was editing the docs to reflect this, I realised that if we do that we will lose a use case: when you want to compare two strings (or dict, tuple) directly without having to call semver.parse.
I wonder if it would be acceptable to lose this functionality for the sake of cleaner code 🤔

@tomschr maybe we could simply change these functions to use the builtins behind the scenes?

I don't want to go about deprecating things and then someone comes up and says that the new workflow is unacceptable and that we need to change it back.

@tomschr maybe we could simply change these functions to use the builtins behind the scenes?

Do you mean to use max (or min) inside max_ver/min_ver? Yes, why not. Do you have an idea?

I don't want to go about deprecating things and then someone comes up and says that the new workflow is unacceptable and that we need to change it back.

Sure. 👍 Maybe we should combine it with documentation?

Do you mean to use max (or min) inside max_ver/min_ver? Yes, why not. Do you have an idea?

I think using map like you suggested with *args would be relatively efficient to implement an arbitrary number of arguments for this function. It would be backwards compatible too 😉.
With some type checking we can pretty easily implement converting from strings, tuples and dictionaries.

Sorry, it was probably too late and I was too tired when I wrote my last comment. My bad, sorry.

After rethinking it a bit, I need to correct and amend my comment. Maybe we need to clarify what we want to do with the max_ver/min_ver functions in general. We started by declaring both functions as deprecated. That's a good start. 👍

If I'm not mistaken, we have the following options:

1. Don't change either function signatures nor implementations.
2. Don't change function signatures, but change implementations (e.g. using the code that I've suggested with map and max).
3. Change function signatures and allow more than two arguments. That means probably, we need to change the implementations too.

Solution 1 would be obviously the least destructive and most compatible. It would also mean we could focus more on documentation.

Solution 2 is something in the middle. On the other hand, why should we change a deprecated function and add functionality before we remove it? As we don't change the function signature, we have only two arguments. Changing the implementation doesn't add much value.

Solution 3 is also similar to solution 2, but changes the function signature. I'm a bit concerned about adding functionality to an already deprecated function.

So all in all, max_ver/min_ver is mostly used for old code.

After thinking about all the options above, weight the pros and cons... I'm leaning towards the first solution. Maybe we should leave the function "as is", but focus more on improving the documentation.

As we declared both functions as deprecated, we shouldn't add additional functionality to it. Especially when we favor the builtins max/min as a better solution for new code.

So to summarize it, my idea would be:

• We don't change the implementation of both deprecated functions "as is" (of course with the @deprecated decorator intact).
• Enhance the docstrings of both functions and add a .. deprecated:: directive as shown in format_version, for example.
• Enhance section "Getting Minimum and Maximum of two Versions":
  • Insert a deprecation warning after the title and warn user about the deprecated functions max_ver/min_ver and the replacement.
  • Add an example of the "old way" with max_ver/min_ver.
  • Add a short text that the preferred way would be to use the builtins max/min in combination with a VersionInfo object.
  • Mention to convert strings into VersionInfo object first with map.
  • Make reader aware that the method with max/min works best when all versions are of the same type.
  • Anything else? (Maybe pros and cons between "old" and "new" way?)

What do you think? Would that be a meaningful approach?

I'm a bit concerned about adding functionality to an already deprecated function.

Actually the PR (#264 ) to deprecate these functions hasn't been merged yet, so these functions aren't deprecated yet 😉.

As such, the current state of these functions makes them not very useful. Improving could have potential to make them more useful, but if we want to stick to the bare essentials and keep the fluff at a minimum, then maybe the best way to go is to deprecate these functions and make the documentation super clear about how you can use the VersionInfo thanks to it's magic functions.

If I am feeling the vibe of this project right, I think we want to keep it simple, yet powerful.
As such, these are the only two public module functions left that have not been deprecated. I think this might be a sign that we should focus more on having a powerful class that is well documented than having a bunch of utility functions that are probably little used shortcuts for use cases that we can fulfill simply by knowing what VersionInfo is really capable of 💪.

What do you think? Would that be a meaningful approach?

Definitely 😃

I'm a bit concerned about adding functionality to an already deprecated function.

Actually the PR (#264 ) to deprecate these functions hasn't been merged yet, so these functions aren't deprecated yet 😉.

Haha, yes, that's true. But almost. 😉

As such, the current state of these functions makes them not very useful.

That's also my feeling. Or maybe someone else comes up that it indeed fulfill a certain need.

[...]
If I am feeling the vibe of this project right, I think we want to keep it simple, yet powerful.
As such, these are the only two public module functions left that have not been deprecated. I think this might be a sign that we should focus more on having a powerful class that is well documented than having a bunch of utility functions that are probably little used shortcuts for use cases that we can fulfill simply by knowing what VersionInfo is really capable of 💪.

I agree with you. Probably having a good and powerful VersionInfo class does make things easier. A lot of of these comparisons cannot be done with pure version strings, for example.

There was some concern in another issue that some shortcuts (like semver.compare) are shorter to write. That could may be the case. Not sure how to handle this case yet.

What do you think? Would that be a meaningful approach?

Definitely smiley

👍