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.

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

Jump to bottom

Improve natural sort #29611

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Improve natural sort #29611

merged 3 commits into from
Mar 5, 2024

Conversation

wxiaoguang
Copy link
Contributor

Hugely simplify the code, and add more tests (only new approach could pass)

@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Mar 5, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Mar 5, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 5, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 5, 2024
@wxiaoguang wxiaoguang mentioned this pull request Mar 5, 2024
Open
3 tasks
@silverwind
Copy link
Member

silverwind commented Mar 5, 2024 •
edited
Loading

How does the current algorithm compare to that package in terms of memory consumption/speed? Would it make sense to just depend on that package?

If current implementation is kept, maybe add some test cases from https://github.com/maruel/natural as well.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 5, 2024 •
edited
Loading

How does the current algorithm compare to that package in terms of memory consumption/speed? Would it make sense to just depend on that package?

  1. I would like to trust the official Golang package, and it has already been used.
  2. The "NaturalSortLess" is not on the hot path. The code around it is much slower than it. So there should be no performance problem.
    • It is only used by "file list", when the files are being listed, other functions/methods are much more slower.

image

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 5, 2024 •
edited
Loading

If current implementation is kept, maybe add some test cases from https://github.com/maruel/natural as well.

maruel/natural doesn't work and won't pass the tests, because it doesn't handle "unicode"

And I think the current tests are good enough.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 5, 2024
@silverwind
Copy link
Member

silverwind commented Mar 5, 2024 •
edited
Loading

If current implementation is kept, maybe add some test cases from https://github.com/maruel/natural as well.

maruel/natural doesn't work and won't pass the tests, because it doesn't handle "unicode"

And I think the current tests are good enough.

Right, if it doesn't even support unicode, it's a no-go.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 5, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 5, 2024
@silverwind silverwind enabled auto-merge (squash) March 5, 2024 14:35
@silverwind silverwind merged commit ebff37a into go-gitea:main Mar 5, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 5, 2024
@wxiaoguang wxiaoguang deleted the fix-natural-sort branch March 5, 2024 15:31
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 6, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
810335f 
* giteaofficial/main:
  Run editorconfig-checker on `locale_en-US.ini` (go-gitea#29608)
  bump protobuf module (go-gitea#29617)
  Add ac claim for old docker/build-push-action@v3 / current buildx gha cache (go-gitea#29584)
  Skip email domain check when admins edit user emails (go-gitea#29609)
  Improve natural sort (go-gitea#29611)
  Add empty repo check in `DetectAndHandleSchedules` (go-gitea#29606)
  Fix contributor graphs mobile layout and responsiveness (go-gitea#29597)
  Skip email domain check when admin users adds user manually (go-gitea#29522)
  Replace more `gt-` with `tw-`, update frontend docs (go-gitea#29595)
  Remove unnecessary ctxData for "attachments" template (go-gitea#29600)
  Adjust tailwind content globs (go-gitea#29596)
  Use flex wrap to layout the PR update button (go-gitea#29590)
  Make "/user/login" page redirect if the current user has signed in (go-gitea#29583)
  [skip ci] Updated translations via Crowdin
  Fix projects mode bugs (go-gitea#29593)
  Regenerate fomantic lockfile and build it with our browserslist (go-gitea#29560)
  Do not exceed display for the PR page buttons on smaller screens (go-gitea#29418)
  Add aria-label to the navbar menu button (go-gitea#29587)
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 6, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into overflow-menu
de5074c 
* origin/main: (29 commits)
  Tweak actions color and borders (go-gitea#29640)
  Add download URL for executable files (go-gitea#28260)
  Move all login and account creation page labels to be above inputs (go-gitea#29432)
  Avoid issue info panic (go-gitea#29625)
  Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444)
  Avoid unexpected panic in graceful manager (go-gitea#29629)
  Add a link for the recently pushed branch notification (go-gitea#29627)
  Fix wrong header of org project view page (go-gitea#29626)
  Detect broken git hooks (go-gitea#29494)
  Sync branches to DB immediately when handle git hook calling (go-gitea#29493)
  Fix wrong line number in code search result (go-gitea#29260)
  Make wiki default branch name changable (go-gitea#29603)
  A small refactor for agit implementation (go-gitea#29614)
  Update Twitter Logo (go-gitea#29621)
  Fix 500 error when adding PR comment (go-gitea#29622)
  Run editorconfig-checker on `locale_en-US.ini` (go-gitea#29608)
  bump protobuf module (go-gitea#29617)
  Add ac claim for old docker/build-push-action@v3 / current buildx gha cache (go-gitea#29584)
  Skip email domain check when admins edit user emails (go-gitea#29609)
  Improve natural sort (go-gitea#29611)
  ...
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wxiaoguang @silverwind @KN4CK3R @delvh @GiteaBot