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

explore: adopt anyhow, support CustomValue, remove help system #12692

Merged
merged 9 commits into from May 1, 2024

Conversation

rgwood
Copy link
Contributor

@rgwood rgwood commented Apr 28, 2024

This PR:

  1. Adds basic support for CustomValue to explore. Previously open foo.db | explore didn't really work, now we "materialize" the whole database to a Value before loading it
  2. Adopts anyhow for error handling in explore. Previously we were kind of rolling our own version of anyhow by shoving all errors into a std::io::Error; I think this is much nicer. This was necessary because as part of 1), collecting input is now fallible...
  3. Removes a lot of explore's fancy command help system.
    • Previously each command (:help, :try, etc.) had a sophisticated help system with examples etc... but this was not very visible to users. You had to know to run :help :try or view a list of commands with :help :
    • As discussed previously, we eventually want to move to a less modal approach for explore, without the Vim-like commands. And so I don't think it's worth keeping this command help system around (it's intertwined with other stuff, and making these changes would have been harder if keeping it).
  4. Rename the --reverse flag to --tail. The flag scrolls to the end of the data, which IMO is described better by "tail"
  5. Does some renaming+commenting to clear up things I found difficult to understand when navigating the explore code

I initially thought 1) would be just a few lines, and then this PR blew up into much more extensive changes 😅

Before

The whole database was being displayed as a single Nuon/JSON line 🤔
image

After

The database gets displayed like a record
image

Future work

It is sort of annoying that we have to load a whole SQLite database into memory to make this work; it will be impractical for large databases. I'd like to explore improvements to CustomValue that can make this work more efficiently.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

One sec, I added a few expect()s during development that I need to fix up...

@rgwood rgwood marked this pull request as draft April 28, 2024 03:14
@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

OK, I bit off more than I can chew in 1 evening. Making collect_input() fallible (b/c CustomValue::to_base_value() can fail) has a lot of knock-on effects. Will hopefully revisit this soon.

@rgwood rgwood changed the title explore improvements: adopt anyhow, support CustomValue explore: adopt anyhow, support CustomValue, remove help system Apr 28, 2024
@rgwood rgwood marked this pull request as ready for review April 28, 2024 15:49
@fdncred
Copy link
Collaborator

fdncred commented Apr 28, 2024

@rgwood Thanks for cleaning this up more. I'm anxious to play with the new sqlite functionality.

I do have some questions though. If I'm following the code correctly, which I may not be, it seems that we've gone from hard-to-find help to impossible to find help or no help at all.

What I mean by that are these things. Granted, you had to know where to look to find help before, but now, I'm not sure how you know these exist.

expand (e)
Shortcode::new("Up",        "",     "Moves the viewport one row up"),
Shortcode::new("Down",      "",     "Moves the viewport one row down"),
Shortcode::new("Left",      "",     "Moves the viewport one column left"),
Shortcode::new("Right",     "",     "Moves the viewport one column right"),
Shortcode::new("PgDown",    "",     "Moves the viewport one page of rows down"),
Shortcode::new("PgUp",      "",     "Moves the cursor or viewport one page of rows up"),
Shortcode::new("Esc",       "",     "Exits cursor mode. Exits the currently explored data."),

table (t)
Shortcode::new("Up",     "",        "Moves the cursor or viewport one row up"),
Shortcode::new("Down",   "",        "Moves the cursor or viewport one row down"),
Shortcode::new("Left",   "",        "Moves the cursor or viewport one column left"),
Shortcode::new("Right",  "",        "Moves the cursor or viewport one column right"),
Shortcode::new("PgDown", "view",    "Moves the cursor or viewport one page of rows down"),
Shortcode::new("PgUp",   "view",    "Moves the cursor or viewport one page of rows up"),
Shortcode::new("Esc",    "",        "Exits cursor mode. Exits the just explored dataset."),
Shortcode::new("i",      "view",    "Enters cursor mode to inspect individual cells"),
Shortcode::new("t",      "view",    "Transpose table, so that columns become rows and vice versa"),
Shortcode::new("e",      "view",    "Open expand view (equivalent of :expand)"),
Shortcode::new("Enter",  "cursor",  "In cursor mode, explore the data of the selected cell"),

:try
Shortcode::new("Up",     "",        "Switches between input and a output panes"),
Shortcode::new("Down",   "",        "Switches between input and a output panes"),
Shortcode::new("Esc",    "",        "Switches between input and a output panes"),
Shortcode::new("Tab",    "",        "Switches between input and a output panes"),

Is there a plan to augment the help screen or something to show people what commands and shortcuts/aliases do?

Speaking of which, seems like the aliases aren't discoverable either like :q!, :nu, I think :q and :try may still be on the initial page though.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

If I'm following the code correctly, which I may not be, it seems that we've gone from hard-to-find help to impossible to find help or no help at all.

You are following the code correctly! I think we're better off with no help than what was there before. I really, really want to simplify the explore code and I think a lot of what we had was unnecessary. Ideally a single page of info in the :help command would be enough to explain the essentials, especially while the UX for explore is in flux.

I can add some more info to the :help page (like e for expand and t for transpose, maybe a bit more info about other shortcuts). Anything in particular you'd like added?

Speaking of which, seems like the aliases aren't discoverable either like :q!, :nu, I think :q and :try may still be on the initial page though.

Is the :nu command still worth keeping around when we have :try and are hoping to get rid of the Vim-inspired command system? Can I drop it in the interest of simplifying things? I suspect it has very few users since it was not mentioned in :help.

Likewise, is it important that we document+support both :q and :q!?

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

Here's my stab at adding more info to :help:
image

@fdncred
Copy link
Collaborator

fdncred commented Apr 28, 2024

I think saying that most screens support Up, Down, Left, Right, PgUp, PgDown and maybe a blurb about Esc is probably enough for navigation. Tables have Enter, i, t, and e. Maybe a short description maybe for those would help? I'm fine with all this being on the first screen you see when you launch explore or if you hit ? on some popup.

I don't see :nu or :try being vim-ish. The value in :nu is just being able to type an entire pipeline and view it in explore versus the limited try ui.

RE: :q and :q!. These are very vim-ish. I added :q! because I kept typing it and it didn't quit. I like the ability to repetively hit Esc and eventually get out, but there's some value to typing :q from anywhere and just exiting right there, and now. I could live with :q or :quit alone.

I think your new help screen is a step in the right direction.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

I'm fine with all this being on the first screen you see when you launch explore

Hmm, good idea. Right now the info on the first screen (the InformationView) is very limited:

image

Maybe we should display all the info from :help on that screen instead of a limited subset?

@fdncred
Copy link
Collaborator

fdncred commented Apr 28, 2024

Maybe we should display all the info from :help on that screen instead of a limited subset?

Yes, I agree. The challenge comes in that when you do ls | explore and nothing is shown, how do we show it when a user needs it? Ideally, ? would show the same screen somehow.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

When you do ls | explore it shows this in the bottom left which I think is good enough for now:

image

@fdncred
Copy link
Collaborator

fdncred commented Apr 28, 2024

ok, I'm fine with that for now then.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

How's this (as both the help page and the landing page when opening explore without any input)?

image

I can make it nicer with styling later, just don't have time right now to wrap my head around what explore is doing for styled text (seems like a complicated hybrid between Ratatui's text styling and Nushell's styles).

@fdncred
Copy link
Collaborator

fdncred commented Apr 28, 2024

The only thing that isn't mentioned is i.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 28, 2024

Yeah, idk… I am really not a Vim person and i rubs me the wrong way when Enter does the same and more. But if you think it’s important to document I will.

Edit: to elaborate, i is a Vim-ism for "entering insert mode", and in explore we've decided that the mode where you have a cell selected is kinda like insert mode in Vim. But to me i feels unnecessary+confusing because Enter can enter that mode and drill down into cells. I think Enter and Esc are what we should be primarily explaining to users; Enter goes 1 direction and Esc goes the other, no need to think about modes. It's not a hill I want to die on, but if it were up to me I wouldn't bother explaining i.

@rgwood
Copy link
Contributor Author

rgwood commented Apr 29, 2024

Help page with formatting:
image

@fdncred
Copy link
Collaborator

fdncred commented Apr 29, 2024

If Enter does the same as i then I have no problem with not mentioning it. I was just going from the old help to make sure everything was covered.

@fdncred
Copy link
Collaborator

fdncred commented May 1, 2024

@rgwood are you ready to land this?

@rgwood
Copy link
Contributor Author

rgwood commented May 1, 2024

Yes! Good to go if we're far enough away from the release to merge things 👍

@fdncred fdncred merged commit 3d34065 into nushell:main May 1, 2024
16 checks passed
@hustcer hustcer added this to the v0.94.0 milestone May 2, 2024
foo.db Show resolved Hide resolved
rgwood added a commit that referenced this pull request May 2, 2024
Remove a small (20kb) SQLite database that was accidentally added as
part of #12692
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

4 participants