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

VIM-2615 add support to sort u command #638

Merged
merged 1 commit into from
May 29, 2023

Conversation

samabcde
Copy link
Contributor

@samabcde samabcde commented May 9, 2023

  1. support sort u
  2. fix natural sort issue when both string not contain number
  3. refactored SortCommandTest

Copy link
Contributor

@lippfi lippfi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

@AlexPl292
Copy link
Member

Hi there, thank you for your contribution.
I'm not sure, will it work fine with the i flag and two lines with the same word of different cases? I guess, distinct won't eliminate such lines, will it?
I've tried to find this case in test cases, but looks like it's not tested. Something like:

content = """
data
DATA
""",
command = "sort ui",
expected = "data",

@samabcde
Copy link
Contributor Author

samabcde commented May 19, 2023

Hi there, thank you for your contribution. I'm not sure, will it work fine with the i flag and two lines with the same word of different cases? I guess, distinct won't eliminate such lines, will it? I've tried to find this case in test cases, but looks like it's not tested. Something like:

content = """
data
DATA
""",
command = "sort ui",
expected = "data",

@AlexPl292 Thx for pointing out. Yes, you are correct that distinct won't eliminate such lines.
I have updated the PR:

  1. Implemented unique with case insensitive handling in ChangeGroup#sortTextRange
  2. Corresponding tests are SortCommandTest#caseInsensitiveUniqueSortTestCases and SortCommandTest#numericCaseInsensitiveReverseUniqueSortTestCases
    Please review again.

@AlexPl292
Copy link
Member

Okay, this seems to be a better solution. Glad to see this supported, thank you for your PR!

@AlexPl292 AlexPl292 merged commit 4e7149c into JetBrains:master May 29, 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.

3 participants