Skip to content

update command: make $in(in closure body) takes cell path#8610

Merged
sholderbach merged 4 commits intonushell:mainfrom
WindSoilder:update_command
Apr 15, 2023
Merged

update command: make $in(in closure body) takes cell path#8610
sholderbach merged 4 commits intonushell:mainfrom
WindSoilder:update_command

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

Description

Make $in takes cell path in update command

The reason behind the change: https://discord.com/channels/601130461678272522/615329862395101194/1088405671080370196

when i use update on some cell path, it's almost always because i want to start with its previous value and change it.

cc @amtoine

User-Facing Changes

Before

open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| $in.metadata.binstall.pkg-fmt | str replace "g" "FOO"}

After

open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| str replace "g" "FOO"}

If use want to access original raw, it can be accessed by parameters in closure:

open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|$it| $it.metadata.binstall.pkg-fmt | str replace "g" "FOO"}

For this reason, I don't think we need to add a flag like --whole

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

Note
from nushell you can also use the toolkit as follows

use toolkit.nu  # or use an `env_change` hook to activate it automatically
toolkit check pr

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@WindSoilder WindSoilder changed the title update command: make $in in the closure body takes cell path update command: make $in(in closure body) takes cell path Mar 25, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2023

Codecov Report

Merging #8610 (17338fa) into main (0567407) will increase coverage by 0.36%.
The diff coverage is 100.00%.

❗ Current head 17338fa differs from pull request most recent head 20a240f. Consider uploading reports for the commit 20a240f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8610      +/-   ##
==========================================
+ Coverage   68.16%   68.53%   +0.36%     
==========================================
  Files         631      631              
  Lines      101963   101965       +2     
==========================================
+ Hits        69502    69878     +376     
+ Misses      32461    32087     -374     
Impacted Files Coverage Δ
crates/nu-command/src/filters/update.rs 96.89% <100.00%> (+0.05%) ⬆️

... and 10 files with indirect coverage changes

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 25, 2023

@WindSoilder

that is a really elegant solution 😮
way better than a --whole option!

thanks a lot ⭐

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 25, 2023

so this will probably break commands like

open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| get metadata.binstall.pkg-fmt | str replace "g" "FOO"}

, right? 😱

Note
this is the structure i use the most 'cause i did not want to abuse with $in 👀

@WindSoilder
Copy link
Copy Markdown
Contributor Author

Yeah, it's a breaking change..

@WindSoilder WindSoilder added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Mar 25, 2023
@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 25, 2023

Yeah, it's a breaking change..

ehe no worries 😋

that's the life i've chosen to live 😏

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Apr 1, 2023

@WindSoilder - can you add some tests?

@WindSoilder WindSoilder marked this pull request as draft April 2, 2023 15:02
@WindSoilder
Copy link
Copy Markdown
Contributor Author

@jntrnr Yeah, but currently I found some strange behavior about the following command:

[[project, authors]; ['nu', ['Andrés', 'JT', 'Yehuda']]] | update authors {|| $in | str join ','}

It doesn't update authors to 'Andrés, JT, Yehuda', mark as draft for now.

@WindSoilder WindSoilder marked this pull request as ready for review April 3, 2023 08:00
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 5, 2023

I've wanted it to work like this for a long time. I wonder if upsert or insert need this as well? 🤔

@WindSoilder
Copy link
Copy Markdown
Contributor Author

Oh, I just get one thought....For insert, does it make sense to follow cell path on $in? Because the column we want to insert is not exists in $in, the same thing happened on upsert command.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 5, 2023

Maybe not on insert but remember that upsert is supposed to be an update or an insert. So, the update part should work.

@sholderbach
Copy link
Copy Markdown
Member

Maybe not on insert but remember that upsert is supposed to be an update or an insert. So, the update part should work.

I agree with @WindSoilder, we can not assume that the column is already present in upsert

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Apr 6, 2023

Consider the following example:
[{'b': 1}, {'a': 3}] | upsert a {|| into string }
For the first row doesn't contain column a, what's the proper value of $in?

@WindSoilder
Copy link
Copy Markdown
Contributor Author

Hi, I have a discuss with @fdncred , and think it's ok to leave upsert command un-changed, and this pr only affects update command, so I think it's ready :-)

@sholderbach
Copy link
Copy Markdown
Member

Ok let's give it a try! Thanks

@sholderbach sholderbach merged commit cbedc84 into nushell:main Apr 15, 2023
bobhy pushed a commit to bobhy/nushell that referenced this pull request Apr 15, 2023
…l#8610)

# Description

Make `$in` takes cell path in `update` command

The reason behind the change:
https://discord.com/channels/601130461678272522/615329862395101194/1088405671080370196
> when i use update on some cell path, it's almost always because i want
to start with its previous value and change it.

cc @amtoine 

# User-Facing Changes

## Before
```
open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| $in.metadata.binstall.pkg-fmt | str replace "g" "FOO"}
```

## After
```
open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| str replace "g" "FOO"}
```

If use want to access original raw, it can be accessed by parameters in
closure:
```
open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|$it| $it.metadata.binstall.pkg-fmt | str replace "g" "FOO"}
```
For this reason, I don't think we need to add a flag like `--whole`

# Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
@WindSoilder WindSoilder deleted the update_command branch August 2, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants