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

use to_lowercase in str downcase #10850

Merged
merged 2 commits into from Oct 27, 2023
Merged

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Oct 27, 2023

Description

as we can see in the documentation of str.to_lowercase, not only ASCII symbols have lower and upper variants.

this PR uses str.to_lower_case instead of str.to_ascii_lowercase in str downcase.

User-Facing Changes

  • upcase still works fine
~ l> "ὀδυσσεύς" | str upcase
ὈΔΥΣΣΕΎΣ
  • downcase now works

👉 before

~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὈΔΥΣΣΕΎΣ

👉 after

~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὀδυσσεύς

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • toolkit test
  • toolkit test stdlib

adds two tests

  • non_ascii_upcase
  • non_ascii_downcase

After Submitting

@amtoine amtoine marked this pull request as ready for review October 27, 2023 09:56
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

I think this is technically a breaking change, but aren't all bug fixes breaking changes?

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Tell me muse, of the deeds of the Unicode which travelled to many scripts after the destruction of U+FFFD

ὀδυσσεύς

@amtoine
Copy link
Member Author

amtoine commented Oct 27, 2023

I think this is technically a breaking change, but aren't all bug fixes breaking changes?

i would say every fix is a repearing-change rather than a breaking-change 😉

@amtoine
Copy link
Member Author

amtoine commented Oct 27, 2023

let's go with this then 🙏

@amtoine amtoine merged commit 01d8961 into nushell:main Oct 27, 2023
19 checks passed
@amtoine amtoine deleted the fix-ascii-str-downcase branch October 27, 2023 17:16
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description
as we can see in the [documentation of
`str.to_lowercase`](https://doc.rust-lang.org/std/primitive.str.html#method.to_lowercase),
not only ASCII symbols have lower and upper variants.

- `str upcase` uses the correct method to convert the string

https://github.com/nushell/nushell/blob/7ac5a01e2f5020a1295697edab727cd62a5844b7/crates/nu-command/src/strings/str_/case/upcase.rs#L93
- `str downcase` incorrectly converts only ASCII characters

https://github.com/nushell/nushell/blob/7ac5a01e2f5020a1295697edab727cd62a5844b7/crates/nu-command/src/strings/str_/case/downcase.rs#L124

this PR uses `str.to_lower_case` instead of `str.to_ascii_lowercase` in
`str downcase`.

# User-Facing Changes
- upcase still works fine
```nushell
~ l> "ὀδυσσεύς" | str upcase
ὈΔΥΣΣΕΎΣ
```
- downcase now works

:point_right: before
```nushell
~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὈΔΥΣΣΕΎΣ
```
:point_right: after
```nushell
~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὀδυσσεύς
```

# Tests + Formatting
- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :black_circle: `toolkit test`
- :black_circle: `toolkit test stdlib`

adds two tests
- `non_ascii_upcase`
- `non_ascii_downcase`

# After Submitting
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
as we can see in the [documentation of
`str.to_lowercase`](https://doc.rust-lang.org/std/primitive.str.html#method.to_lowercase),
not only ASCII symbols have lower and upper variants.

- `str upcase` uses the correct method to convert the string

https://github.com/nushell/nushell/blob/7ac5a01e2f5020a1295697edab727cd62a5844b7/crates/nu-command/src/strings/str_/case/upcase.rs#L93
- `str downcase` incorrectly converts only ASCII characters

https://github.com/nushell/nushell/blob/7ac5a01e2f5020a1295697edab727cd62a5844b7/crates/nu-command/src/strings/str_/case/downcase.rs#L124

this PR uses `str.to_lower_case` instead of `str.to_ascii_lowercase` in
`str downcase`.

# User-Facing Changes
- upcase still works fine
```nushell
~ l> "ὀδυσσεύς" | str upcase
ὈΔΥΣΣΕΎΣ
```
- downcase now works

:point_right: before
```nushell
~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὈΔΥΣΣΕΎΣ
```
:point_right: after
```nushell
~ l> "ὈΔΥΣΣΕΎΣ" | str downcase
ὀδυσσεύς
```

# Tests + Formatting
- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :black_circle: `toolkit test`
- :black_circle: `toolkit test stdlib`

adds two tests
- `non_ascii_upcase`
- `non_ascii_downcase`

# After Submitting
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.

None yet

3 participants