Skip to content
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

"cmp" function should either be documented or removed from the API #117

Closed
rydrman opened this issue Feb 13, 2019 · 3 comments
Closed

"cmp" function should either be documented or removed from the API #117

rydrman opened this issue Feb 13, 2019 · 3 comments
Labels
Doc Documentation related issue Release_3.x.y Only for the major release 3
Projects

Comments

@rydrman
Copy link

rydrman commented Feb 13, 2019

This function is part of the documentation and public API but is undocumented and looks a little like an alias to compare, though it is very much not. I'm simply suggesting that it become _cmp or get a useful docstring.

@doransmestad
Copy link

Just got burned by this. Was using cmp when I should have been using compare. If anyone else runs into an issue where semver comparison isn't working correctly, check that you're using the compare function before doing much else debugging.

The current documentation makes it seem like these two functions are the same because of the visual proximity:

image

I haven't looked into the code to tell what the difference between the two are, but perhaps simply removing cmp from the docs will discourage its use?

@tomschr tomschr added Release_3.x.y Only for the major release 3 Doc Documentation related issue labels Dec 17, 2019
@tomschr tomschr added this to To do in Release 3 Dec 17, 2019
versat added a commit to versat/cppcheck that referenced this issue Jan 16, 2020
There were two issues:
1. The version was not correctly extracted out of the filename. When
extracting a sub-string in Python one has to specify start index and
end index instead of start index and length.
2. The function `semver.cmp()` does nothing useful. Instead the function
`semver.compare()` must be called when two version should be compared.
See python-semver/python-semver#117 (comment)

Because `semver.compare()` now really compares the versions it is
possible that an exception is thrown if a version is not in the semver
version format. In such cases the sorting is aborted and the last
filename in the array is returned. This is often but not always already
the latest version from what I have seen.
versat added a commit to danmar/cppcheck that referenced this issue Jan 16, 2020
…2490)

There were two issues:
1. The version was not correctly extracted out of the filename. When
extracting a sub-string in Python one has to specify start index and
end index instead of start index and length.
2. The function `semver.cmp()` does nothing useful. Instead the function
`semver.compare()` must be called when two version should be compared.
See python-semver/python-semver#117 (comment)

Because `semver.compare()` now really compares the versions it is
possible that an exception is thrown if a version is not in the semver
version format. In such cases the sorting is aborted and the last
filename in the array is returned. This is often but not always already
the latest version from what I have seen.
@tomschr
Copy link
Member

tomschr commented Apr 26, 2020

Thanks @rydrman @doransmestad for the repot and hints.

A (limited) docstring is available in master now, see https://github.com/python-semver/python-semver/blob/master/semver.py#L27

I haven't looked into the code to tell what the difference between the two are, but perhaps simply removing cmp from the docs will discourage its use?

Acutally, it's coming from Python2 which has a cmp function, but Python3 removed this. So this is just bringing it back. I've added the Python2 docstring. However, I agree, it could be documented better.

Currently it acts as a helper function for the VersionInfo.compare function. Maybe we should better make it "private" by adding a "_" prefix for cmp (=> _cmp).

@tomschr
Copy link
Member

tomschr commented May 15, 2020

perhaps simply removing cmp from the docs will discourage its use?

Yes, I think that is the way to go.

I've merged #242 lately which also has an impact on Sphinx. There, I didn't include semver.cmp as it gives too much confusion. Actually, I consider it as an implementation detail.

Use the official section Comparing Versions in our documentation.

I think, we can close this issue for the time being. If you still have the feeling there is something missing, reopen it and we can discuss it.

Thanks! 👍

@tomschr tomschr closed this as completed May 15, 2020
Release 3 automation moved this from To do to Done May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc Documentation related issue Release_3.x.y Only for the major release 3
Projects
No open projects
Release 3
  
Done
Development

No branches or pull requests

3 participants