-
Notifications
You must be signed in to change notification settings - Fork 169
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
Allow sorting by negative numbers #507
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #507 +/- ##
==========================================
+ Coverage 88.70% 88.72% +0.02%
==========================================
Files 25 25
Lines 2062 2066 +4
==========================================
+ Hits 1829 1833 +4
Misses 233 233 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch, just a little question.
@@ -1 +1,2 @@ | |||
Order: 01 | |||
Order: -10 | |||
PartialOrder: 01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PartialOrder key doesn't seem to be used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't set that, then the PartialOrder test fails:
settings["albums_sort_reverse"] = False
a = Album("dir1", settings, album["subdirs"], album["medias"], gal)
a.sort_subdirs(["meta.partialorder", "meta.order"])
> assert [d.name for d in a.albums] == list(["test1", "test2", "test3"])
E AssertionError: assert ['test2', 'test1', 'test3'] == ['test1', 'test2', 'test3']
E At index 0 diff: 'test2' != 'test1'
E Use -v to get more diff
tests/test_gallery.py:251: AssertionError
I suppose I could have reordered the test assertion, but it's a good check that it's using PartialOrder instead of Order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I missed that !
Sounds good them, thanks again.
One of my users noticed that negative numbers specified in meta ordering weren't being sorted correctly. This adds
ns.SIGNED
to the sorting, as well as modifying the tests to show it works.