Skip to content

Change str replace to match substring by default#10038

Merged
kubouch merged 1 commit intonushell:mainfrom
kubouch:str-replace-default
Aug 17, 2023
Merged

Change str replace to match substring by default#10038
kubouch merged 1 commit intonushell:mainfrom
kubouch:str-replace-default

Conversation

@kubouch
Copy link
Copy Markdown
Contributor

@kubouch kubouch commented Aug 17, 2023

Description

This PR makes str replace to match substring by default instead of regular expression. I was constantly getting tripped over this when using str replace on Windows paths which got interpreted as escape characters.

User-Facing Changes

  • Deprecates str replace --string (this is now the default behavior)
  • Puts regex matching behind --regex flag. (--multiline flag implies --regex)

Tests + Formatting

After Submitting

@kubouch kubouch added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Aug 17, 2023
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 17, 2023

100% agree that dealing with Windows paths is a huge pain because of escaping.

Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

this looks good and sensible to me 👍

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 17, 2023

It seems like most commands have regex as an option like --regex/-r except for !~ and =~ which are regex by default. If there are other commands that use regex, we should probably be consistent and use the --regex/-r. So, I +1 this for consistency.

Although, there is some weirdness going on in oh-my.nu. I guess I'll have to research it.
This PR
image
Before this PR
image

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 17, 2023

It seems like most commands have regex as an option like --regex/-r except for !~ and =~ which are regex by default. If there are other commands that use regex, we should probably be consistent and use the --regex/-r. So, I +1 this for consistency.

Although, there is some weirdness going on in oh-my.nu. I guess I'll have to research it. This PR image Before this PR image

some scripts might have to be changed to use --regex now when not using --string 👍

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 17, 2023

I had to change statements like this

let home = ($nu.home-path | str replace -a '\\' '/')

to this

let home = ($nu.home-path | str replace -a '\' '/')

or

let home = ($nu.home-path | str replace -ar '\\' '/')

This is going to break a bunch of stuff since things work differently now. 82 occurrences in nu_scripts.

I guess it's not really deprecated since it doesn't work the way it used to?

@kubouch
Copy link
Copy Markdown
Contributor Author

kubouch commented Aug 17, 2023

Yeah, it's a breaking change. Not sure how to avoid that. If we added only the warning for --string, but kept it working as before, there would be no way to get substring matching without the warning spam.

Copy link
Copy Markdown
Contributor

@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'm fine with moving forward with this because it makes str replace more consistent with the other commands that use regular expression parameters. However, we should be prepared for a fairly significant breaking change. str replace is used all over the place.

@kubouch
Copy link
Copy Markdown
Contributor Author

kubouch commented Aug 17, 2023

OK, let's try.

@kubouch kubouch merged commit 2e0fb7c into nushell:main Aug 17, 2023
@kubouch kubouch deleted the str-replace-default branch August 17, 2023 21:18
@kubouch kubouch added the category:deprecation Related to the deprecation of commands/features/options label Aug 18, 2023
@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 18, 2023

this also makes it more consistent with other commands such as find or parse which have --regex as an option, but not --string 👌

amtoine added a commit to amtoine/nu-git-manager that referenced this pull request Aug 18, 2023
amtoine added a commit to amtoine/scripts that referenced this pull request Aug 18, 2023
related to nushell/nushell/pull/10038

commands used
```nushell
sd "str replace (.*) --string" "str FOO replace $1" **/*.nu
sd "str replace" "str replace --regex" **/*.nu
sd "str FOO replace" "str replace" **/*.nu
```
@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 18, 2023

@kubouch
as you can see in the two commits of mine above, i've used

sd "str replace (.*) --string" "str FOO replace $1" **/*.nu
sd "str replace" "str replace --regex" **/*.nu
sd "str FOO replace" "str replace" **/*.nu

to fix the two repos where i had str replace.

i think this is worth putting in the release notes 😋
either with sd or sed or something else 😉

@amtoine amtoine added the notes:mention Include the release notes summary in the "Hall of Fame" section label Aug 18, 2023
amtoine added a commit to amtoine/zellij-layouts that referenced this pull request Aug 18, 2023
related to nushell/nushell/pull/10038

commands used
```nushell
sd "str replace (.*) --string" "str FOO replace $1" **/*.nu
sd "str replace" "str replace --regex" **/*.nu
sd "str FOO replace" "str replace" **/*.nu
```
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 18, 2023

@amtoine I'm not a big fan of pointing people to other tools if nushell can do the same thing via a pipeline. Granted, sd is meant for this singular purpose.

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Aug 18, 2023

@amtoine I'm not a big fan of pointing people to other tools if nushell can do the same thing via a pipeline. Granted, sd is meant for this singular purpose.

regardless of the tool, i think we should give some commands for folks to fix their scripts and config 😋

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Aug 18, 2023

What I did was replace str replace -a blah blah with str replace -ar blah blah and it worked fine. This allowed me to not have to change any parameters but there are more scenarios that that one. That's just the only one I used.

sholderbach pushed a commit that referenced this pull request Sep 19, 2023
related to
- #10038

# Description
`str replace --string` has been deprecated in
#10038 and should be removed
before 0.85.

this PR removes the `--string` option from `str replace` completely.

# User-Facing Changes
`str replace --string` will no longer work and will give an error
instead of a warning.
fnordpig pushed a commit to fnordpig/nushell that referenced this pull request Sep 19, 2023
related to
- nushell#10038

# Description
`str replace --string` has been deprecated in
nushell#10038 and should be removed
before 0.85.

this PR removes the `--string` option from `str replace` completely.

# User-Facing Changes
`str replace --string` will no longer work and will give an error
instead of a warning.
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
related to
- nushell#10038

# Description
`str replace --string` has been deprecated in
nushell#10038 and should be removed
before 0.85.

this PR removes the `--string` option from `str replace` completely.

# User-Facing Changes
`str replace --string` will no longer work and will give an error
instead of a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:deprecation Related to the deprecation of commands/features/options notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes notes:mention Include the release notes summary in the "Hall of Fame" section

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants