use int compare when both version fields are ints#140
use int compare when both version fields are ints#140benprew wants to merge 9 commits intodonadigo:masterfrom
Conversation
src/Package.vala
Outdated
| } else if (a_num > b_num) { | ||
| return 1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
for the strings "32.17.3" and "32.17.10" the method will return 0 and this is the culprit. Remove it and add a if(i == length-1) like in the original code
src/Package.vala
Outdated
| @@ -282,19 +282,19 @@ public class Eddy.Package : Object { | |||
|
|
|||
| int length = int.max (aparts.length, bparts.length); | |||
There was a problem hiding this comment.
this is not a problem coincidentally because you have return 0, but if you do it correctly it will cause out of array error, replace it with int.min and do the length comparison as above
src/Package.vala
Outdated
| bool b_is_num = int.try_parse(bparts[i], out b_num); | ||
|
|
||
| if (a_is_num && b_is_num) { | ||
| if (a_num < b_num) { |
There was a problem hiding this comment.
if(a_num != b_num)
return a_num - b_num;
is also faster for cpu than comparisons
|
We should diligently adhere to the version comparison specification, as described on the Debian site, see https://www.debian.org/doc/debian-policy/ch-controlfields.html#version. It's not only about the upstream version split by |
camilajenny
left a comment
camilajenny
left a comment
There was a problem hiding this comment.
I will create a pr to your code when I'm ready
src/Package.vala
Outdated
| } | ||
| return 0; | ||
| } else { | ||
| return strcmp (aparts[i], bparts[i]); |
There was a problem hiding this comment.
this changes the contract as strcmp doesn't have to return either -1, or 1 but any negative or positive number (cmp. below)
src/Package.vala
Outdated
|
|
||
| int rc = compare_versions (package.get_version (), version); | ||
| int rc = compare_versions (package.get_version (), version); | ||
| if (rc == 1) { |
adhere to Debian version comparison guidelines
|
Update to support different debian version schemes, with tests! Thanks camilajenny! |
|
@donadigo Are you able to review this PR? Are you still supporting eddy? Thanks |
2 participants
Fixes #129
Previous and after my change
