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
add "default" table theme #11072
add "default" table theme #11072
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh having both impl Default
and a Default
variant is pushing the details what the actual Default
is down the stack.
If we could get away with just FromStr
using Default::default
for "default"
it would be more obvious that rounded
is the default.
@@ -11,6 +11,7 @@ pub enum TableMode { | |||
Compact, | |||
WithLove, | |||
CompactDouble, | |||
Default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to move the #[default]
annotation to change the impl Default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these changes in #11076
call.get_flag(state, stack, "theme")? | ||
let theme_name = call.get_flag(state, stack, "theme")?; | ||
theme_name | ||
.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the temporary and clone if we rearrange the .map_err
into the .map
before the .transpose
@@ -60,6 +62,7 @@ impl ReconstructVal for TableMode { | |||
TableMode::Compact => "compact", | |||
TableMode::WithLove => "with_love", | |||
TableMode::CompactDouble => "compact_double", | |||
TableMode::Default => "rounded", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it reports "rounded"
anyways, you could just drop TableMode::Default
entirely and only add the line "default" => Ok(TableMode::default())
to the impl FromStr
. Then there is no chance for that to get out of step in nu-table
's load_theme()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, did that too. i sincerely hope you're out of ideas of other things to change here. 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm done 😄
# Description This PR fixes a minor bug that prevented this command from running. ```nushell table --list | each {|r| print ($r); print (ls | first 3 | table --theme $r)} ``` Here's the output now of the first few themes. ![image](https://github.com/nushell/nushell/assets/343840/21bc8942-5106-4b6a-8905-e90d6cb9a153) It prevented it from running because "default" wasn't a real table theme. Now "default" is a synonym of rounded. Also tweaked the error message when a bad theme name is provided. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking 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` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **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. -->
# Description This PR fixes a minor bug that prevented this command from running. ```nushell table --list | each {|r| print ($r); print (ls | first 3 | table --theme $r)} ``` Here's the output now of the first few themes. ![image](https://github.com/nushell/nushell/assets/343840/21bc8942-5106-4b6a-8905-e90d6cb9a153) It prevented it from running because "default" wasn't a real table theme. Now "default" is a synonym of rounded. Also tweaked the error message when a bad theme name is provided. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking 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` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **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. -->
Description
This PR fixes a minor bug that prevented this command from running.
Here's the output now of the first few themes.
It prevented it from running because "default" wasn't a real table theme. Now "default" is a synonym of rounded.
Also tweaked the error message when a bad theme name is provided.
User-Facing Changes
Tests + Formatting
After Submitting