Skip to content

Update crossterm version to 0.26#8623

Merged
sholderbach merged 14 commits intonushell:mainfrom
WindSoilder:crossterm_ver
Apr 14, 2023
Merged

Update crossterm version to 0.26#8623
sholderbach merged 14 commits intonushell:mainfrom
WindSoilder:crossterm_ver

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Mar 26, 2023 •
edited
Loading

Description

WARN: do not merge this yet, it's using my fork of reedline and lscolors

This pr is a companion to nushell/reedline#560

Fortunally, we don't need to change too much nushell code.

Additional note about lscolor dependency

Currently lscolor is using 0.25 version, I've submitted a pr to update it: sharkdp/lscolors#58
lscolor is using 0.26 for now

User-Facing Changes

None

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.

@fdncred fdncred marked this pull request as draft March 26, 2023 12:06
@WindSoilder WindSoilder marked this pull request as ready for review April 12, 2023 22:15
@WindSoilder WindSoilder marked this pull request as draft April 12, 2023 22:15
@WindSoilder WindSoilder marked this pull request as ready for review April 13, 2023 22:23
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2023 •
edited
Loading

Codecov Report

Merging #8623 (15748bb) into main (8ddebcb) will decrease coverage by 0.43%.
The diff coverage is 7.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8623      +/-   ##
==========================================
- Coverage   68.57%   68.14%   -0.43%     
==========================================
  Files         635      635              
  Lines      101423   101529     +106     
==========================================
- Hits        69548    69189     -359     
- Misses      31875    32340     +465
Impacted Files Coverage Δ
crates/nu-cli/src/commands/keybindings_listen.rs 18.18% <0.00%> (-1.43%) ⬇️
crates/nu-cli/src/repl.rs 3.01% <0.00%> (ø)
crates/nu-command/src/viewers/griddle.rs 81.67% <0.00%> (-1.33%) ⬇️
crates/nu-command/src/viewers/table.rs 83.79% <0.00%> (+0.16%) ⬆️
crates/nu-explore/src/pager/mod.rs 0.00% <ø> (ø)
crates/nu-command/src/filters/find.rs 77.02% <100.00%> (ø)

... and 4 files with indirect coverage changes

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 14, 2023

@WindSoilder are we ready to land this? I'd like to get some good testing in before the release if possible.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Apr 14, 2023

Yeah I think so, I have done some manually testing and think it's ok

sholderbach
sholderbach reviewed Apr 14, 2023
View reviewed changes
Cargo.toml
nu-utils = { path = "./crates/nu-utils", version = "0.78.1" }

nu-ansi-term = "0.47.0"
reedline = { version = "0.18.0", features = ["bashisms", "sqlite"] }
Copy link
Copy Markdown
Member

@sholderbach sholderbach Apr 14, 2023

Choose a reason for hiding this comment

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

Can you revert the manual override outside the patch section? (see L163)

Copy link
Copy Markdown
Contributor Author

@WindSoilder WindSoilder Apr 14, 2023

Choose a reason for hiding this comment

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

It's done, would you please have a look again? Not really sure if I'm doing right thing

sholderbach
sholderbach reviewed Apr 14, 2023
View reviewed changes
crates/nu-cli/Cargo.toml
nu-color-config = { path = "../nu-color-config", version = "0.78.1" }

nu-ansi-term = "0.47.0"
reedline = { version = "0.18.0", features = ["bashisms", "sqlite"] }
Copy link
Copy Markdown
Member

@sholderbach sholderbach Apr 14, 2023

Choose a reason for hiding this comment

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

Same

Copy link
Copy Markdown
Contributor Author

@WindSoilder WindSoilder Apr 14, 2023

Choose a reason for hiding this comment

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

Hi, I get a warning message:
warning: patch for the non root package will be ignored, specify patch at the workspace root

It seems that we don't need to change nu-cli/Cargo.toml.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Apr 14, 2023 •
edited
Loading

oh.....ci freeze on pulling tool chain...Can anyone help me to rerun CI job?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 14, 2023

I'm scared to push the button. 😆

@sholderbach sholderbach merged commit 9b35d59 into nushell:main Apr 14, 2023
@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Apr 14, 2023

Thanks for all your work on this @WindSoilder!

bobhy pushed a commit to bobhy/nushell that referenced this pull request Apr 15, 2023
@WindSoilder @bobhy
Update crossterm version to 0.26 (nushell#8623)
f0836d9 
# Description

This pr is a companion to nushell/reedline#560

Fortunally, we don't need to change too much nushell code.

## Additional note about lscolor dependency
sharkdp/lscolors#58
lscolor is using 0.26 for now
@WindSoilder WindSoilder deleted the crossterm_ver branch August 2, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sholderbach sholderbach sholderbach left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@WindSoilder @fdncred @rgwood @sholderbach