-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: vortex-tui #1911
feat: vortex-tui #1911
Conversation
b985a11
to
324a3f5
Compare
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.
Very nice! It would be good to file follow up issues that whatever introspection you're doing here is probably useful to exist in an easier API, e.g. traversing ArrayData and LayoutData trees.
vortex-cli/Cargo.toml
Outdated
[dependencies] | ||
bytes = { workspace = true } | ||
clap = { version = "4", features = ["derive"] } | ||
crossterm = "0.28" |
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.
Should we move these into root Cargo.toml and use {workspace = true}
?
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.
Yea I was on the fence about this one. The "rusty" thing to do for a binary crate inside of a workspace is seems to be put all deps that are specific to that bin in the local Cargo.toml
E.g. datafusion-cli: https://github.com/apache/datafusion/blob/main/datafusion-cli/Cargo.toml
uv seems to use workspace deps though: https://github.com/astral-sh/uv/blob/main/crates/uv-cli/Cargo.toml
Can move things to workspace
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.
Oh I see, yeah that may make sense given they end up being locked differently right?
/// A pointer into the `Layout` hierarchy that can be advanced. | ||
/// | ||
/// The pointer wraps an InitialRead. | ||
pub struct LayoutCursor { |
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.
Is this somewhat similar to ArrayVisitor?
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.
Somewhat? It's specifically for layout hierarchy, and just gives us an owned pointer that we can mutate to move up/down the tree
As we change/break format makes it easier to keep this tool up to date. Also helpful for debugging/exploring format.
I've also added a
vx
alias so we can immediately access it from cargo, e.g.