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

fix: set repl_buffer_state to the REPL buffer after the `pre_execut… #8560

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

stevenxxiu
Copy link
Contributor

…ion` hook

Description

Previously when a executehostcommand shortcut calls commandline, to get the current command line, it's incorrectly set to the value of executehostcommand cmd.

This fixes the regression (due to #8207), so it's correctly set to what's in the REPL buffer.

User-Facing Changes

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.

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #8560 (e618615) into main (93e5d8e) will increase coverage by 0.39%.
The diff coverage is 92.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8560      +/-   ##
==========================================
+ Coverage   68.13%   68.52%   +0.39%     
==========================================
  Files         624      624              
  Lines      100512   100607      +95     
==========================================
+ Hits        68481    68944     +463     
+ Misses      32031    31663     -368     
Impacted Files Coverage Δ
crates/nu-cli/src/repl.rs 3.01% <0.00%> (ø)
crates/nu-command/src/formats/to/nuon.rs 88.32% <97.70%> (+5.08%) ⬆️
crates/nu-command/src/bytes/length.rs 87.14% <100.00%> (ø)
crates/nu-command/src/filters/uniq.rs 98.22% <100.00%> (ø)

... and 4 files with indirect coverage changes

@fdncred
Copy link
Collaborator

fdncred commented Mar 22, 2023

Thanks for fixing this

@fdncred fdncred merged commit b9858ea into nushell:main Mar 22, 2023
@stevenxxiu stevenxxiu deleted the fix/commandline-get-repl-buffer branch March 22, 2023 12:01
@fdncred
Copy link
Collaborator

fdncred commented Mar 22, 2023

btw - this works soooo much better now. loving it.

@fdncred
Copy link
Collaborator

fdncred commented Mar 22, 2023

@stevenxxiu quick question. Is atuin cross platform? I was looking at the autin.nu PR and it would be nice to do something more nushell-y here than using sh.

def _atuin_search_cmd [...flags: string] {
    [
        $ATUIN_KEYBINDING_TOKEN,
        ([
            `commandline (sh -c 'RUST_LOG=error atuin search `,
            $flags,
            ` -i -- "$0" 3>&1 1>&2 2>&3' (commandline) | str substring ',-1')`,
        ] | flatten | str join ''),
    ] | str join '; '

}

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Mar 23, 2023

@stevenxxiu quick question. Is atuin cross platform? I was looking at the autin.nu PR and it would be nice to do something more nushell-y here than using sh.

I think it is but am not sure, since there's some merged PR on Windows support.

I only use Linux, but would be quite happy to avoid using sh here too, just to be cleaner. The only thing sh does here is switch stdout and stderr. I can't currently do this in Nushell due to the issues:

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