Deprecate max_ver and min_ver
#264
Conversation
There was a problem hiding this comment.
Thanks @tlaferriere, this is great! 👍 You even fix my bad typos. 😄 Thank you! 👍
Apart from the minor suggestions below, I have only one small wish in regards to documentation. My idea would be to distinguish two use cases (explained with maximum/highest values, the same applies for minimum/lowest values of course too):
-
Getting the highest version number from
VersionInfoinstances
=> we don't needmax_ver, we can use the builtinmax. -
Getting the highest version number from version strings
=> we needmax_ver. Additionally, we could mention an alternative withmaplike this:str(max(map(semver.VersionInfo.parse, ['1.1.0', '1.2.0', '2.1.0', '0.5.10', '0.4.99'])))
I would suggest to create (maybe?) a list which describe these two use cases.
However, there is one subtle thing: the different signatures between max and max_ver. Currently, the latter accepts only two versions whereas max can be feed with arbitrary numbers of versions.
If the developer has more than two versions to compare, the documentation should mention the alternative method with map.
Maybe we should fix the signature of max_ver/ min_ver and allow arbitrary numbers of versions to compare. However, I think, this is probably out of scope for this PR. Better we keep this PR short.
|
I forget to add another idea: could you add the two functions into the "Replacing Deprecated Functions" section? See file |
Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
I am fairly confident this is correct and I think it should bedazzle any native English speaker with an appreciation of literature.
… min. Keep the old way of doing for reference.
0547aba to
48f92c6
Compare
Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
|
@tlaferriere Could you add the issue and pr number in the * :gh:`160` (:pr:`264`): Output DeprecationWarning for ``semver.max_ver`` and ``semver.min_ver``.(Just an example, if you have better wording feel free to change it.) |
There was a problem hiding this comment.
Just found a minor thing in the documentation.
Furthermore, I think, we should test the two deprecated functions if they raise a DeprecationWarning. Could you add an example into the test_should_raise_deprecation_warnings function? Shouldn't be very complicated, just two lines.
After that, I think, we can merge it to master. 👍
Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
Sure! While we're at it, I find that the name |
You can read my mind! 😆 After I've wrote my message, I thought the same but didn't change it, unfortunately. So, yes please! Go ahead and use Deprecations. 👍 |
|
I also went ahead and removed empty sections in the changelog, figured it would look cleaner like this. |
|
Thank you Thomas. Much appreciated! 👍 As I've mentioned in #264 (comment), we should add the two deprecated functions also in the section Replacing Deprecated Functions. The list is sorted, so I'd suggest to add it between I think, that will complete it and we can merge it to master. 😉 |
|
Oh, by the way: I plan to release 2.10.2 when this is merged. |
|
I'm out for a few hours, if there are any more typos go ahead and fix them so you can merge this soon. |
|
@tlaferriere! I think, it's in a state now that we can merge it to master. Haven't found any further issues. Thank you very much for all your efforts! 👍 Will merge it. |
This could close #160 .
As I said in this comment, this PR will deprecate functions that could be useful as a shortcut.
As you can see in the new examples in the documentation, it is quite a mouthful to find the maximum of two version strings.
As an alternative to this we could keep
max_verandmin_veras utility functions that automatically convert from whatever supported types we have, passes them tomaxorminthen converts back into the input types to return them.