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

to html --list now returns a table #7080

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Conversation

webbedspace
Copy link
Contributor

@webbedspace webbedspace commented Nov 10, 2022

Description

I noticed that to html --list produced the bare minimum output, so I decided it needed to give structured data.

Note: I am aware to html's themes don't actually work at all. If this is accepted, I plan to do a PR afterward to improve its output a tad.

P.S: The message Screenshots of themes can be found here: https://github.com/mbadolato/iTerm2-Color-Schemes has been moved to the help message.

Example output (edit: colours removed for use in a forthcoming PR)

image

I'm aware this is only particularly useful on wide screens, but then again, there isn't really a more compact way to render all of this. And, there's always | get name when you just want the names.

Major Changes

If you're considering making any major change to nushell, before starting work on it, seek feedback from regular contributors and get approval for the idea from the core team either on Discord or GitHub issue.
Making sure we're all on board with the change saves everybody's time.
Thanks!

Tests + Formatting

Make sure you've done the following, if applicable:

  • Add tests that cover your changes (either in the command examples, the crate/tests folder, or in the /tests folder)
    • Try to think about corner cases and various ways how your changes could break. Cover those in the 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 tests pass

After Submitting

  • Help us keep the docs up to date: If your PR affects the user experience of Nushell (adding/removing a command, changing an input/output type, etc.), make sure the changes are reflected in the documentation (https://github.com/nushell/nushell.github.io) after the PR is merged.

@fdncred
Copy link
Collaborator

fdncred commented Nov 10, 2022

oh, wow. that is cool!

@webbedspace
Copy link
Contributor Author

webbedspace commented Nov 10, 2022

Thinking about it, I want to split this into two PRs, one where any string exactly matching the six-hex color format gets auto-coloured in this way by table, not just tied to this one specific table.

@fdncred fdncred marked this pull request as draft November 10, 2022 16:04
@fdncred
Copy link
Collaborator

fdncred commented Nov 10, 2022

i did something similar, but different, in ansi --list

@webbedspace
Copy link
Contributor Author

Ah yes, thanks for the reminder.

Thinking some more, I've decided to bundle the six-hex color format coloring idea with a PR explicitly implementing #6909.

@fdncred
Copy link
Collaborator

fdncred commented Nov 10, 2022

We may need to talk more about your particular design plan for this. I'm all for making it better but since I authored a bunch of the color stuff in nushell, I may be picky. But I 100% agree it can be better and is honestly kind of a mess right now. So, before you spend hours coding, I'd prefer you to kind of lay out the plan in discord or in an issue so we can chat about it as a team, especially if there will be breaking changes.

@webbedspace webbedspace marked this pull request as ready for review November 10, 2022 16:23
@webbedspace
Copy link
Contributor Author

OK, I'll do that as soon as I can.

@sholderbach sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Nov 11, 2022
@fdncred
Copy link
Collaborator

fdncred commented Nov 13, 2022

@webbedspace were you thinking of landing this, or do you want to close it and rethink it based on #6909 ?

@webbedspace
Copy link
Contributor Author

I think it's worth landing as-is, even without the extra razzle and/or dazzle of the color_config closures PR.

@fdncred
Copy link
Collaborator

fdncred commented Nov 15, 2022

I think it's worth landing as-is, even without the extra razzle and/or dazzle of the color_config closures PR.

That's what I was thinking too.

Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

LGTM ok, i take it back. I tested again and i'm not getting the highlights now, i'm not sure why.

@webbedspace
Copy link
Contributor Author

webbedspace commented Nov 15, 2022

I took the highlights out in b3a9522 because the "correct" way to enable them, as I said, is to use pull/7141 with string: { if $in =~ '^#\w{6}$' { $in } else { 'white' } }. (Even though I say in that PR description that I don't recommend computed styles for string at the moment, I'm wondering if the perf issues could be worked out somehow.)

Again, I think the data is still valuable in list format without the highlighting.

That being said, if you really want me to put it back, I could.

@fdncred
Copy link
Collaborator

fdncred commented Nov 15, 2022

my personal preference would be to have the highlights in there, even if it's not doing it in the absolute perfect way. If we don't want to do it this way, then i'd advocate to closing this PR until we can get other things in order first before trying to do this. For me, the list of a zillion hex colors without seeing the actual colors really looks cluttered and doesn't provide a lot of real help.

@webbedspace
Copy link
Contributor Author

OK, it's back in.

@fdncred
Copy link
Collaborator

fdncred commented Nov 15, 2022

ok, looks much better now. +1 to land it.
image

@dandavison
Copy link
Contributor

LGTM!

@fdncred fdncred merged commit f856e64 into nushell:main Nov 15, 2022
@fdncred
Copy link
Collaborator

fdncred commented Nov 15, 2022

Thanks again @webbedspace!!

@webbedspace webbedspace deleted the html-theme-list branch November 16, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants