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

feat: apply the --numbered option to acc in reduce command. #5575

Merged
merged 2 commits into from
May 18, 2022

Conversation

jaeheonji
Copy link
Contributor

@jaeheonji jaeheonji commented May 18, 2022

Description

The idea for this PR comes from the commands::reduce::reduce_numbered_example test in #4314.

I was curious about the --numbered option of reduce after seeing the following tests:

// FIXME: jt: needs more work
#[ignore]
#[test]
fn reduce_numbered_example() {
    let actual = nu!(
        cwd: ".", pipeline(
        r#"
        echo one longest three bar
        | reduce -n { |it, acc| if ($it.item | str length) > ($acc | str length) {echo $it.item} else {echo $acc}}
        | get index
        "#
        )
    );

    assert_eq!(actual.out, "1");
}

One of the reasons this test cannot work is that the types returned by it and acc are different.
Currently, the -n option converts only the it argument to a block containing an index.

This is why, in other examples, the item property is accessed only in the it.
It is a little bit different to the regular reduce provided in other languages.

So I modified the -n option to apply to both acc and it.

In this case, the test will get the result we expect if we change it like this:

> echo one longest three bar | reduce -n { |it, acc| if ($it.item | str length) > ($acc.item | str length) { echo $it } else { echo $acc } }
╭───────┬─────────╮
│ index │ 1       │
│ item  │ longest │
╰───────┴─────────╯
> 〉echo one longest three bar | reduce -n { |it, acc| if ($it.item | str length) > ($acc.item | str length) { echo $it } else { echo $acc } } | get index
1

Of course, this is a proposal. So, the related tests and examples have not been updated yet.
If we can come to an agreement on this, I will update the rest of the content, including the tests.

Tests

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 --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

@fdncred
Copy link
Collaborator

fdncred commented May 18, 2022

I like the proposal

@jaeheonji jaeheonji changed the title feat: apply the --numered option to acc in reduce command. feat: apply the --numbered option to acc in reduce command. May 18, 2022
@jaeheonji
Copy link
Contributor Author

I updated it. If possible, please review it 😸

@fdncred
Copy link
Collaborator

fdncred commented May 18, 2022

lgtm, thanks.

@fdncred fdncred merged commit 9c779b0 into nushell:main May 18, 2022
fennewald pushed a commit to fennewald/nushell that referenced this pull request Jun 27, 2022
…ell#5575)

* feat: apply the `-n` option to acc

* feat: update tests and examples
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

2 participants