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 option to sort base on certain locale. #1818

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

SirZenith
Copy link
Contributor

@SirZenith SirZenith commented Oct 25, 2024

This pull request adds implementation for sorting with user provided locale (Fixes #323).

An new option locale is introduced, which has default value ''. And this option supports setlocal.

  • When this option is set to empty string, localized sorting is disabled, and program should behave the same as before.
  • When set to certain IETF BCP 47 language tag, file list will be sort with cultural habits of specified language.
  • Special value * is used to indicate reading locale value from environment.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution.

I think it is a good idea to support locale-aware sorting, but I would prefer to implement it in a way that doesn't introduce too much complexity. I have left some comments which you might want to look at, although I personally don't have much experience with using the collate library.

Also note that there is a change freeze for a couple of weeks while I am planning to make a new release, so this PR will have to wait until after that point to be merged.

doc.md Outdated Show resolved Hide resolved
doc.md Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
nav.go Outdated Show resolved Hide resolved
misc.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying your expectations about how locale should work. I've gone through the code in a bit more detail this time, but this should probably be the last set of comments and I think the PR looks much better overall.

Also FYI you can add something like Fixes #323 to the PR description, which will link the issue properly and automatically close it when the PR is merged.

doc.md Outdated Show resolved Hide resolved
doc.md Outdated Show resolved Hide resolved
eval.go Outdated Show resolved Hide resolved
misc.go Outdated Show resolved Hide resolved
misc.go Outdated Show resolved Hide resolved
misc.go Outdated Show resolved Hide resolved
misc.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks, I think it looks good now. I have given my approval, but will hold off merging it until after the freeze period is over.

@joelim-work joelim-work merged commit 3f63bc9 into gokcehan:master Nov 1, 2024
4 checks passed
@joelim-work joelim-work added the new Pull requests that add new behavior label Nov 1, 2024
@joelim-work joelim-work added this to the r34 milestone Nov 1, 2024
@joelim-work joelim-work mentioned this pull request Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new Pull requests that add new behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request - sort by locale
2 participants