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

Add the 'none' sorting setting #655

Merged
merged 4 commits into from
Jul 7, 2023
Merged

Conversation

buzzingwires
Copy link
Contributor

@buzzingwires buzzingwires commented Jul 7, 2023

Starting with what I consider to be one of the simplest changes discussed in #651 and as always, nothing is as simple as expected.

It works pretty much as discussed, but it's worth noting that I found reversing the sort order does not work, probably because lambda x: 0 doesn't provide the sorting algorithm varied numbers to meaningfully reverse. I toyed around with ways of making reversing work, but figured I'd stay with the agreed upon approach of keeping this setting simple for now.


For sort 'image_order' and 'directory_order', the 'none' setting can be used to allow images to follow the last order they were encountered in by the software, instead of re-sorting.

Note that setting another sorting mode while the software is running then switching back to 'none' will see files remain in the previous sort order. The original order of images is not tracked.

Additionally, 'none' sort cannot be reversed.

For sort 'image_order' and 'directory_order', the 'none' setting
can be used to allow images to follow the last order they were
encountered in by the software, instead of re-sorting.

Note that setting another sorting mode while the software is running
then switching back to 'none' will see files remain in the previous
sort order. The original order of images is *not* tracked.

Additionally, 'none' sort cannot be reversed.
@buzzingwires buzzingwires marked this pull request as ready for review July 7, 2023 05:12
@karlch
Copy link
Owner

karlch commented Jul 7, 2023

Thanks, with tests, documentation and everything ❤️

Reverse is a good catch. I do think it should support reversing, what about you? If we want to change this, we could pass a simple incrementing count as key, here is what I tested with:

class Incrementer:  # Could go into utils

    def __init__(self):
        self._count = 0

    def __call__(self, _):
        self._count += 1
        return self._count


inc = Incrementer()  # Would have to be passed instead of the lambda
x = [5, 1, 3, 0, 2]

print(sorted(x))
print(sorted(x, key=inc))
print(sorted(x, key=inc, reverse=True))

or maybe you came up with something even simpler when toying around?

Cosmetics:

  • Feel free to add yourself to the AUTHORS file
  • Would you mind adding this to docs/changelog.rst, ideally thanking yourself 😄

@buzzingwires
Copy link
Contributor Author

You're welcome!

And, reversing would be nice to have, but at least for my use case, not super-important. Another issue is that setting reverse to false doesn't set the order back to normal. It just naively passes the reversed values back through the sorting function. That's part of the reason I didn't go any further with trying to implement reversing.

The approach I took was just to call reversed separately, if reversing was enabled. Something like:

    def sort(self, values: Iterable[str]) -> List[str]:
        """Sort values according to the current ordering."""
        ordering = self._get_ordering()
        output = sorted(values, key=ordering)
        if sort.reverse.value:
            output = list(reversed(output))
        return output

There might still need to be some additions to the sort function either way, and I'm not sure which would be better performance-wise, if the performance difference is significant, anyway.

And sure! Do you prefer I just add commits to the pull request or to amend and force push?

@karlch
Copy link
Owner

karlch commented Jul 7, 2023

That is a valid point, it will not properly reverse back as there is no sorting order, so I suppose having it documenting as you did is the best approach. Maybe you could add a sentence saying that the main use-case is to keep a custom input order from the commandline?

Just add some new commits 😊

Specify that it is mostly used for keeping the order from the
command line or stdin. Try to make the phrasing slightly more concise.
@buzzingwires
Copy link
Contributor Author

Got it! Made the changes described. Hopefully it doesn't bloat the size of the table the options are in- my explanation is so incongruously long.

@karlch
Copy link
Owner

karlch commented Jul 7, 2023

Mini-nitpick in the changelog, otherwise LGTM!

Table is fine with me 😊
scrot-2023-07-07_14:00:40

@karlch karlch merged commit 0f39a55 into karlch:master Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants