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 heck for string casing #4081

Merged
merged 1 commit into from Dec 14, 2021
Merged

use heck for string casing #4081

merged 1 commit into from Dec 14, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2021

I removed the Inflector dependency in favor of heck for two reasons:

We could probably alter the structure of the str_ module to expose the
individual casing behaviors better.
I did not feel as confident on changing those signatures.

So I took a lazier approach of a macro in the mod.rs that creates the public
shimming function to heck's traits.

@ghost
Copy link
Author

ghost commented Oct 15, 2021

I also am depending on a commit because the latest stable release does not have Upper Camel Case (our PascalCase).

@fdncred
Copy link
Collaborator

fdncred commented Oct 15, 2021

@efx I'm not sure we can use it if the crate isn't published. I think it prevents us releasing a new release.
heck = { git = "https://github.com/withoutboats/heck.git", rev = "e78e2582770e03dff9d5945fea055350783175d6"}

@ghost
Copy link
Author

ghost commented Oct 15, 2021

@efx I'm not sure we can use it if the crate isn't published. I think it prevents us releasing a new release. heck = { git = "https://github.com/withoutboats/heck.git", rev = "e78e2582770e03dff9d5945fea055350783175d6"}

Ah, thanks for the heads up @fdncred.
I'll reach out to the maintainers of heck to see if they can release something.

@ghost ghost marked this pull request as draft October 15, 2021 18:00
@ghost ghost mentioned this pull request Oct 15, 2021
@sophiajt
Copy link
Contributor

sophiajt commented Dec 2, 2021

@efx - did you hear anything back?

@ghost
Copy link
Author

ghost commented Dec 7, 2021

@jntrnr apologies on the delay! I have not unfortunately. I'll ping again. Do you have a preference for how I manage this open PR? Thanks!

@ghost ghost marked this pull request as ready for review December 13, 2021 14:53
@ghost
Copy link
Author

ghost commented Dec 13, 2021

@jntrnr @fdncred 👋 The upstream work has been merged so disregard my earlier comment. This is ready for review 😄!

@fdncred
Copy link
Collaborator

fdncred commented Dec 13, 2021

not sure what's going on with the CI system but it's puking for no reason. we should wait a few hours and retrigger i think.

@fdncred
Copy link
Collaborator

fdncred commented Dec 13, 2021

can you push an empty commit. I can't restart the ci.

@ghost
Copy link
Author

ghost commented Dec 14, 2021

LOL, it seems like macOS-10.14 is deprecated as of 12/10/2021: Azure/azure-sdk-for-cpp#3168.
I'll open a separate PR for that.

@fdncred
Copy link
Collaborator

fdncred commented Dec 14, 2021

i reran the mac CI and it still said the same errors. you may need to close this and resubmit it since i just landed your CI fix.

I removed the Inflector dependency in favor of heck for two reasons:
- to close #3674.
- heck seems simpler and actively maintained

We could probably alter the structure of the `str_` module to expose the
individual casing behaviors better.
I did not feel as confident on changing those signatures.

So I took a lazier approach of a macro in the `mod.rs` that creates the public
shimming function to heck's traits.
@ghost
Copy link
Author

ghost commented Dec 14, 2021

i reran the mac CI and it still said the same errors. you may need to close this and resubmit it since i just landed your CI fix.

Thank you for running that!

I went ahead and rebased with the latest CI fixes.

@fdncred
Copy link
Collaborator

fdncred commented Dec 14, 2021

ok, good. crossing my fingers.

@fdncred fdncred merged commit e919f9a into nushell:main Dec 14, 2021
@fdncred
Copy link
Collaborator

fdncred commented Dec 14, 2021

finally! thanks @efx

@ghost ghost deleted the close-3674 branch December 14, 2021 15:48
sholderbach pushed a commit that referenced this pull request Oct 13, 2023
Re-fixes #3674, if that is seen as desirable to do.

# Description
This PR changes the implementation of the `--features=extra` string
casing commands from Inflector to `heck`, as in PR #4081. This PR landed
a long time ago, but somewhere along the way (i can't find it) the
implementation ended up being switched back to Inflector.

# User-Facing Changes
Inflector and `heck` implement casing differently, so all of the
commands have different behavior around edge cases (consecutive
capitals, interspersed numbers and letters, etc)

### Before
```nu
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str camel-case
╭───┬───────────╮
│ 0 │ userID    │
│ 1 │ abcdefGHI │
│ 2 │ foo123Bar │
╰───┴───────────╯
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str snake-case
╭───┬─────────────╮
│ 0 │ user_id     │
│ 1 │ ab_cdef_ghi │
│ 2 │ foo_12_3bar │
╰───┴─────────────╯
```

### After
```nu
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str camel-case
╭───┬───────────╮
│ 0 │ userId    │
│ 1 │ abCdefGhi │
│ 2 │ foo123bar │
╰───┴───────────╯
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str snake-case
╭───┬─────────────╮
│ 0 │ user_id     │
│ 1 │ ab_cdef_ghi │
│ 2 │ foo123bar   │
╰───┴─────────────╯
```

# Tests + Formatting

The existing string casing tests pass... because none of them relied on
any of these edge cases
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
Re-fixes nushell#3674, if that is seen as desirable to do.

# Description
This PR changes the implementation of the `--features=extra` string
casing commands from Inflector to `heck`, as in PR nushell#4081. This PR landed
a long time ago, but somewhere along the way (i can't find it) the
implementation ended up being switched back to Inflector.

# User-Facing Changes
Inflector and `heck` implement casing differently, so all of the
commands have different behavior around edge cases (consecutive
capitals, interspersed numbers and letters, etc)

### Before
```nu
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str camel-case
╭───┬───────────╮
│ 0 │ userID    │
│ 1 │ abcdefGHI │
│ 2 │ foo123Bar │
╰───┴───────────╯
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str snake-case
╭───┬─────────────╮
│ 0 │ user_id     │
│ 1 │ ab_cdef_ghi │
│ 2 │ foo_12_3bar │
╰───┴─────────────╯
```

### After
```nu
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str camel-case
╭───┬───────────╮
│ 0 │ userId    │
│ 1 │ abCdefGhi │
│ 2 │ foo123bar │
╰───┴───────────╯
G:/Dev/nu-itself/nushell> [UserID ABCdefGHI foo123bar] | str snake-case
╭───┬─────────────╮
│ 0 │ user_id     │
│ 1 │ ab_cdef_ghi │
│ 2 │ foo123bar   │
╰───┴─────────────╯
```

# Tests + Formatting

The existing string casing tests pass... because none of them relied on
any of these edge cases
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.

camel case conversion should lowercase abbreviations
2 participants