Skip to content

Improve alphabetical ordering for non-version entries #19

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.

Sign up for GitHub

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

Merged
JamesMGreene merged 14 commits into from
Oct 20, 2021
Merged

Improve alphabetical ordering for non-version entries #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e28beff
Improve test code consistency
JamesMGreene Oct 15, 2021
cab24dd
Add tests for sorting version numbers with v/V prefixes
JamesMGreene Oct 15, 2021
8d253ca
Add test for sorting non-version data with trailing numbers alphabeti…
JamesMGreene Oct 15, 2021
adba5ad
Update test for sorting non-version data to a more alphabetically-inc…
JamesMGreene Oct 15, 2021
6840114
Add test for yui style tags to match benchmark fixtures
JamesMGreene Oct 16, 2021
27ec38a
Update parse_version_number to short-circuit on non-versions
JamesMGreene Oct 16, 2021
eaa0275
Add a test to verify the README example
JamesMGreene Oct 16, 2021
718e23b
Slight ordering expectation adjustment to tests
JamesMGreene Oct 16, 2021
b5babff
Handle special case for zero recognized chunks with alphabetical sorting
JamesMGreene Oct 16, 2021
269b7b6
Undo short-circuiting code in parse_version_number
JamesMGreene Oct 16, 2021
b214d73
Adjust string chunk building logic to include as many hyphenated port…
JamesMGreene Oct 16, 2021
3c8b797
Reduce size comparisons from 2 to 1
JamesMGreene Oct 16, 2021
d63ec17
Revert stylistic changes to test code
JamesMGreene Oct 18, 2021
489a1a1
Add comments explaining test intent
JamesMGreene Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions ext/version_sorter/version_sorter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ compare_version_number(const struct version_number *a,
}
}

if (max_n == 0) {
return strcmp(a->original, b->original);
}

if (a->size < b->size)
return (b->num_flags & (1ull << n)) ? -1 : 1;

Expand Down Expand Up @@ -149,10 +153,7 @@ parse_version_number(const char *string)
if (string[offset] == '-' || isalpha(string[offset])) {
uint16_t start = offset;

if (string[offset] == '-')
offset++;

while (isalpha(string[offset]))
while (string[offset] == '-' || isalpha(string[offset]))
offset++;

version->comp[comp_n].string.offset = start;
Expand Down
57 changes: 52 additions & 5 deletions test/version_sorter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ def test_sorts_versions_correctly
assert_equal sorted_versions, VersionSorter.sort(versions)
end

def test_sorts_versions_correctly_with_lowercase_prefixes
versions = %w( v1.0.9 v1.0.10 v2.0 v3.1.4.2 v1.0.9a )
sorted_versions = %w( v1.0.9a v1.0.9 v1.0.10 v2.0 v3.1.4.2 )

assert_equal sorted_versions, VersionSorter.sort(versions)
end

def test_sorts_versions_correctly_with_uppercase_prefixes
versions = %w( V1.0.9 V1.0.10 V2.0 V3.1.4.2 V1.0.9a )
sorted_versions = %w( V1.0.9a V1.0.9 V1.0.10 V2.0 V3.1.4.2 )

assert_equal sorted_versions, VersionSorter.sort(versions)
end

def test_sorts_versions_like_rubygems
versions = %w(1.0.9.b 1.0.9 1.0.10 2.0 3.1.4.2 1.0.9a 2.0rc2 2.0-rc1)
if !Gem.respond_to?(:rubygems_version) || Gem.rubygems_version < Gem::Version.new('2.1.0')
Expand All @@ -38,7 +52,7 @@ def test_returns_same_object
end

def test_reverse_sorts_versions_correctly
versions = %w(1.0.9 1.0.10 2.0 3.1.4.2 1.0.9a)
versions = %w( 1.0.9 1.0.10 2.0 3.1.4.2 1.0.9a )
sorted_versions = %w( 3.1.4.2 2.0 1.0.10 1.0.9 1.0.9a )

assert_equal sorted_versions, VersionSorter.rsort(versions)
Expand All @@ -59,18 +73,51 @@ def test_does_not_raise_on_number_overflow

def test_handles_non_version_data
non_versions = [
"", " ", ".", "-", "ćevapčići", "The Quick Brown Fox", '!@#$%^&*()',
" ", ".", "-", "", "ćevapčići", "The Quick Brown Fox", '!@#$%^&*()',
"<--------->", "a12a8a4a22122d01541b62193e9bdad7f5eda552", "1." * 65
]
sorted = [
"<--------->", "-", "The Quick Brown Fox",
"a12a8a4a22122d01541b62193e9bdad7f5eda552", "ćevapčići",
"", " ", ".", '!@#$%^&*()', "1." * 65
"", " ", '!@#$%^&*()', "-", ".", "<--------->",
"The Quick Brown Fox", "a12a8a4a22122d01541b62193e9bdad7f5eda552",
"ćevapčići", "1." * 65
Comment on lines +80 to +82
Copy link
Contributor

@mislav mislav Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that sorting is wildly different here after this change worries me a lot. Was the goal of the PR to refine how these are sorted? If not, would it be possible to only scope the changes in this PR to achieve the desired sorting of specific cases but leave all other behaviors intact?

Copy link
Member Author

@JamesMGreene JamesMGreene Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the trouble of the current sorting mechanism for non-versions can be summed up as:

  • It will [basically] sort as expected for alphanumeric characters
  • But all other characters (or lack thereof), they are effectively ignored, and the output order will be more based on input order

This is why I added the strcmp usage specifically for comparisons with 0 comparison chunks, which will make sorting for those special cases deterministic.

Here's a second PR with a new test (which will fail) demonstrating the issue: #20

P.S. To be clear: there is still opportunity for other special cases that mix alphanumerics and non-alphanumerics to end up sorted non-deterministically. 😬

]

assert_equal sorted, VersionSorter.sort(non_versions)
end

def test_sorts_non_version_data_with_trailing_numbers
non_versions = [
"my-patch-2", "my-patch1", "my-patch2", "my-patch", "my-patch-1"
]
sorted = [
"my-patch", "my-patch1", "my-patch2", "my-patch-1", "my-patch-2"
]

assert_equal sorted, VersionSorter.sort(non_versions)
end

# This verifies the sort order of a subset of `test/tags.txt`
def test_yui_style_tags
yui_tags = [
"yui3-571", "yui3-309", "yui3-1405", "yui3-1537", "yui3-440",
"yui3-572", "yui3-1406", "yui3-1538", "yui3-441", "yui3-573"
]
sorted = [
"yui3-309", "yui3-440", "yui3-441", "yui3-571", "yui3-572",
"yui3-573", "yui3-1405", "yui3-1406", "yui3-1537", "yui3-1538"
]

assert_equal sorted, VersionSorter.sort(yui_tags)
end

# This verifies the sort order of the example in `README.md`
def test_readme_examples
readme_versions = ["1.0.9", "2.0", "1.0.10", "1.0.3", "2.0.pre"]
sorted = ["1.0.3", "1.0.9", "1.0.10", "2.0.pre", "2.0"]

assert_equal sorted, VersionSorter.sort(readme_versions)
end

def test_sort_bang
versions = ["10.0", "1.0", "2.0"]
VersionSorter.sort! versions
Expand Down