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

Fix: Fixed sorting issues with #-# numbered files #17012

Merged
merged 7 commits into from
Apr 8, 2025

Conversation

nethbotheju
Copy link
Contributor

@nethbotheju nethbotheju commented Apr 3, 2025

Resolved / Related Issues
Fixes #17009

Steps used to test these changes
For the test, I have created another single file and copied the NaturalStringComparer code to it and tested the changes.

Screenshot 2025-04-03 181205

@yaira2 yaira2 changed the title Fix: files sorting problems with #-# numbered files Fix: Fixed sorting issues with #-# numbered files Apr 3, 2025
0x5bfa

This comment was marked as outdated.

@dongle-the-gadget
Copy link
Contributor

Potential copyright issue, as NaturalStringComparer is MIT licensed (so copy of license required), but the copyright notice isn't updated properly.

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Confirmed working well!
image

Potential copyright issue, as NaturalStringComparer is MIT licensed (so copy of license required), but the copyright notice isn't updated properly.

I did a quick search but all of pieces of code are fully made by community members only for Files.


FWIW, .NET 10 will bring a numeric comparison feature so this may no longer needed in the future.

@dongle-the-gadget
Copy link
Contributor

Author credits NaturalStringComparer and using GitHub Code Search on that specific repo does indicate a match.

@0x5bfa
Copy link
Member

0x5bfa commented Apr 4, 2025

Can you link me to that repo?

@0x5bfa
Copy link
Member

0x5bfa commented Apr 4, 2025

@nethbotheju @yaira2 we may as well add a license acknowledgement for NaturalStringComparer:

{
	// Credit: https://github.com/GihanSoft/NaturalStringComparer
	public sealed class NaturalStringComparer
	{
	}
}

@nethbotheju
Copy link
Contributor Author

@0x5bfa I have updated the file with the license acknowledgement comment.

Copy link
Contributor

@Lamparter Lamparter left a comment

Choose a reason for hiding this comment

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

There are a couple formatting issues here, otherwise LGTM

Looks like this file had some formatting issues as well before this change was made

image

nethbotheju and others added 2 commits April 4, 2025 23:01
@nethbotheju
Copy link
Contributor Author

nethbotheju commented Apr 4, 2025

@Lamparter Thanks for the formatting suggestions.

@yaira2
Copy link
Member

yaira2 commented Apr 6, 2025

FWIW, .NET 10 will bring a numeric comparison feature so this may no longer needed in the future.

@0x5bfa agreed, this should be a temporary solution. That said, I would like to check with the author before using their code in this manner.

@yaira2
Copy link
Member

yaira2 commented Apr 6, 2025

@GihanSoft thank you for your work on NaturalStringComparer. We're looking to use this in Files without bringing in the entire package. Do you have any objections with the way this is being implemented?

@GihanSoft
Copy link

Sorry being late
Use it as you please
I'll be glad if you mentioned it on readme or about of app but not mandatory.

For advice:

  1. Are you using last version of code? I recently changed entire algorithm. Becoming faster and more memory efficient.

  2. Shouldn't you either use my code in future or use .net-10 source from start to have constance behavior?

@yaira2
Copy link
Member

yaira2 commented Apr 7, 2025

@GihanSoft thank you for the response. We'll be happy to add a notice to our about page.

Are you using last version of code? I recently changed entire algorithm. Becoming faster and more memory efficient.

@nethbotheju can you confirm?

Shouldn't you either use my code in future or use .net-10 source from start to have constance behavior?

.net10 is still in preview so we'll be on .net9 for some time.

@nethbotheju
Copy link
Contributor Author

@yaira2 Yes, it uses the latest version of the code, which was updated 5 months ago

@yaira2
Copy link
Member

yaira2 commented Apr 8, 2025

@nethbotheju can you please add a notice to the 'third party libraries' section on the about page?

@nethbotheju
Copy link
Contributor Author

nethbotheju commented Apr 8, 2025

@yaira2 Are you referring to the https://files.community/docs/features/integrations section of the website?

@dongle-the-gadget
Copy link
Contributor

I think it is NOTICE.md

@nethbotheju
Copy link
Contributor Author

@yaira2 I have updated the NOTICE.md file with the NaturalStringComparer licenses

@yaira2
Copy link
Member

yaira2 commented Apr 8, 2025

@nethbotheju @GihanSoft thank you!

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Apr 8, 2025
@yaira2 yaira2 merged commit 2747d23 into files-community:main Apr 8, 2025
6 checks passed
@nethbotheju nethbotheju deleted the SortProblem#-# branch April 8, 2025 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: sorting problems with #-# numbered files
6 participants