Display empty records and lists #7925

NotLebedev · 2023-01-31T17:11:49Z

Description

Fix some issues related to #7444

Empty lists and records are now displayed as a small notice in a box:
Empty records are now correctly displayed if inside of another record list or table:
Fixed inconsistent coloring of empty list placeholder inside of lists/tables:

User-Facing Changes

table command now displays empty records and lists like a table with text and correctly displays empty records inside tables and lists.

New behavior of displaying empty lists and records can be disabled using table.show_empty config option.

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

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.

NotLebedev · 2023-01-31T17:27:08Z

Spotted an issue. Commands like mkdir and rm that have a --verbose option return empty list even when called without --verbose

Not sure what to do with this

rgwood · 2023-02-07T14:25:02Z

Thank you; this looks like a really good improvement!

For mkdir and rm... yeah, those commands are a bit of a mess, I don't like how they return info as a list<string> instead of printing to stderr.

It would be a breaking change, but maybe we should finally change those to return nothing and print to stderr?

rgwood · 2023-02-07T14:43:15Z

I asked on Discord and other people agree that we should change the filesystem commands to return nothing and print info to stderr: https://discord.com/channels/601130461678272522/615329862395101194/1072523941865857055

Would you be up for making that change (either as part of this PR or in another PR)? Sorry that this is turning into a bit of a yak shave 😭

NotLebedev · 2023-02-07T14:57:49Z

Yes, I can do this. I will make a separate PR. It will be easier to work for me that way

# Description Makes `mkdir`, `cp`, `mv` and `rm` return nothing and print info to stderr: ![image](https://user-images.githubusercontent.com/17511668/217859228-feffa4bc-c22d-45d3-b330-1903f5a4d938.png) See #7925 (comment) and [discord](https://discord.com/channels/601130461678272522/615329862395101194/1072523941865857055). # User-Facing Changes `mkdir`, `cp`, `mv` and `rm` will return nothing and print info to stderr with `--verbose` flag. # 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 # 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.

NotLebedev · 2023-02-10T08:08:42Z

I think a useful addition might be a config option and table command option to switch to old behavior of not displaying this box

I think adding config.table.show_empty option and table --no-empty-placeholder should be useful. This should only control this boxes. This `[list 0 items] will be displayed always

Also, if this option is added, should it be on or off by default?

rgwood · 2023-02-10T22:07:57Z

Also, if this option is added, should it be on or off by default?

I vote for on by default. I don't think it needs to be configurable, but I'm OK with a config option if you want one.

NotLebedev · 2023-02-15T14:38:52Z

I vote for on by default. I don't think it needs to be configurable, but I'm OK with a config option if you want one.

I already did it this way. This PR is ready for review if somebody got time.

rgwood · 2023-02-15T22:02:56Z

Cool!

I haven't had time to do a full code review yet, but I'll try to get to it soon. We discussed this PR in the weekly team meeting, and we agreed that we like the change. It's probably going to break a few things, so the plan is to merge it shortly after the next release (which is on Feb 21) - that will give us more time to test it in main.

rgwood

LGTM. Thank you for the PR!

Looking forward to merging this after the upcoming release.

fdncred · 2023-02-22T12:59:09Z

@NotLebedev would you mind fixing these conflicts so we can merge this pr? Thanks!

fdncred · 2023-02-22T14:13:41Z

I'm wondering if this change is breaking our table tests or just if another merge main is needed since I just landed another table thing?

NotLebedev · 2023-02-22T14:46:58Z

It broke a test. There were missing {record 0 fields} in expected table output

codecov · 2023-02-22T15:16:04Z

Codecov Report

Merging #7925 (85087e4) into main (8608d8d) will decrease coverage by 0.07%.
The diff coverage is 27.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7925      +/-   ##
==========================================
- Coverage   54.28%   54.21%   -0.07%     
==========================================
  Files         608      608              
  Lines       98949    99138     +189     
==========================================
+ Hits        53713    53748      +35     
- Misses      45236    45390     +154

Impacted Files	Coverage Δ
crates/nu-command/src/viewers/table.rs	`31.18% <25.42%> (-0.01%)`	⬇️
crates/nu-protocol/src/ty.rs	`72.85% <28.00%> (-11.50%)`	⬇️
crates/nu-color-config/src/style_computer.rs	`80.76% <100.00%> (ø)`
crates/nu-protocol/src/config.rs	`38.01% <100.00%> (+0.17%)`	⬆️
crates/nu-table/src/nu_protocol_table.rs	`0.00% <0.00%> (ø)`

fdncred · 2023-02-22T16:18:40Z

Let's try it out.

rgwood · 2023-03-05T16:27:31Z

Following up on the discussion in #8014 (comment)

How are we feeling about this one? We're 9 days from the release and it seems like we're still running into related issues. I'm wondering if we should temporarily disable this feature to give us more time to polish it.

It feels like this PR has led to several others like #8292 and I don't think we're in a place where we can confidently explain to users "here's what we changed, and here's why it's better" yet.

NotLebedev · 2023-03-05T18:29:26Z

Following up on the discussion in #8014 (comment)

How are we feeling about this one? We're 9 days from the release and it seems like we're still running into related issues. I'm wondering if we should temporarily disable this feature to give us more time to polish it.

It feels like this PR has led to several others like #8292 and I don't think we're in a place where we can confidently explain to users "here's what we changed, and here's why it's better" yet.

I agree. This and related changes are a bit raw currently. #8292 is sure to cause some more trouble and may bring up some more changes.

This one can be removed from release entirely or disabled by default for this release via table.show_empty config option.

rgwood · 2023-03-07T19:07:12Z

@NotLebedev Do you have time to submit a PR to disable this? If not I'll get around to it later today or tomorrow.

NotLebedev · 2023-03-07T19:10:12Z

@NotLebedev Do you have time to submit a PR to disable this? If not I'll get around to it later today or tomorrow.

Can do. Are we disabling it by default or what should I do?

rgwood · 2023-03-07T19:12:36Z

I don't know what the best thing to do is (haven't had time to think about this much).

hustcer · 2023-03-08T02:21:40Z

To me, the most important thing currently is to make sure [1 2 3] | each {|e| print $e } don't display empty list

# Description Hides #7925 from config and disables by default. Option is still present in config, just hidden. # User-Facing Changes Users can no longer find `table.show_empty` in config and it is set to false by default. # 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 # 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.

NotLebedev added 5 commits January 31, 2023 16:09

Add placeholder for empty records

243bb80

Move record rendering logic into separate function

169956c

Add placeholder for empty lists and list streams

a06684a

Add placeholder for inner record with no fields

73579a5

Fix style_primitive incorrectly selecting style for lists

16b5348

NotLebedev changed the title ~~Empty table placeholder~~ Display empty records and lists Jan 31, 2023

NotLebedev mentioned this pull request Feb 9, 2023

Filesystem commands print --verbose to stderr #8014

Merged

NotLebedev added 4 commits February 9, 2023 21:38

Merge branch 'nushell:main' into empty-table-placeholder

d880e2d

Fix test checking for empty list

05bb97e

Fix test checking for empty list

43367f2

Ignore empty list result to prevent matching part of escape sequence

d609703

NotLebedev added 3 commits February 10, 2023 22:03

Add table.show_empty config option

1cc4cac

Add table.show_empty to default config

1845c44

Use table.show_empty in create_empty_placeholder

6a2bcfa

fdncred added the wait-until-after-nushell-release label Feb 16, 2023

rgwood approved these changes Feb 17, 2023

View reviewed changes

Merge branch 'main' into empty-table-placeholder

b071502

Fix missing empty record text in test

85087e4

fdncred added this pull request to the merge queue Feb 22, 2023

Merged via the queue into nushell:main with commit e389e51 Feb 22, 2023

rgwood mentioned this pull request Mar 1, 2023

Empty records {} and lists [] aren't rendered well in the output #6765

Closed

sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Mar 5, 2023

NotLebedev mentioned this pull request Mar 8, 2023

Hide 7925 #8359

Merged

hustcer modified the milestone: v0.77.0 Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display empty records and lists #7925

Display empty records and lists #7925

NotLebedev commented Jan 31, 2023 •

edited

NotLebedev commented Jan 31, 2023

rgwood commented Feb 7, 2023

rgwood commented Feb 7, 2023

NotLebedev commented Feb 7, 2023

NotLebedev commented Feb 10, 2023 •

edited

rgwood commented Feb 10, 2023

NotLebedev commented Feb 15, 2023

rgwood commented Feb 15, 2023

rgwood left a comment

fdncred commented Feb 22, 2023

fdncred commented Feb 22, 2023

NotLebedev commented Feb 22, 2023

codecov bot commented Feb 22, 2023

fdncred commented Feb 22, 2023

rgwood commented Mar 5, 2023 •

edited

NotLebedev commented Mar 5, 2023

rgwood commented Mar 7, 2023

NotLebedev commented Mar 7, 2023

rgwood commented Mar 7, 2023

hustcer commented Mar 8, 2023

Display empty records and lists #7925

Display empty records and lists #7925

Conversation

NotLebedev commented Jan 31, 2023 • edited

Description

User-Facing Changes

Tests + Formatting

After Submitting

NotLebedev commented Jan 31, 2023

rgwood commented Feb 7, 2023

rgwood commented Feb 7, 2023

NotLebedev commented Feb 7, 2023

NotLebedev commented Feb 10, 2023 • edited

rgwood commented Feb 10, 2023

NotLebedev commented Feb 15, 2023

rgwood commented Feb 15, 2023

rgwood left a comment

Choose a reason for hiding this comment

fdncred commented Feb 22, 2023

fdncred commented Feb 22, 2023

NotLebedev commented Feb 22, 2023

codecov bot commented Feb 22, 2023

Codecov Report

fdncred commented Feb 22, 2023

rgwood commented Mar 5, 2023 • edited

NotLebedev commented Mar 5, 2023

rgwood commented Mar 7, 2023

NotLebedev commented Mar 7, 2023

rgwood commented Mar 7, 2023

hustcer commented Mar 8, 2023

NotLebedev commented Jan 31, 2023 •

edited

NotLebedev commented Feb 10, 2023 •

edited

rgwood commented Mar 5, 2023 •

edited