# Description
The `explore` command is `less`-like, but it's missing the `Emacs`
keybindings for up/down and PageUp/PageDown as well as the "q" to quit
out. When I looked into adding those additional keybindings, I noticed
there was a lot of duplicated code in the various views, so I refactored
the code into a new `trait CursorMoveHandler`. I also noticed that there
was an existing `TODO: should we add a noop transition instead of doing
Option<Transition> everywhere?` comment in the code. I went ahead and
implemented a new `Transition::None`, and that made the new `trait
CursorMoveHandler` code MUCH cleaner, in addition to making some of the
old code a little cleaner as well.
# User-Facing Changes
Users that are used to the keybindings for `less` should feel much more
comfortable using `explore`.
# Tests + Formatting
Unfortunately, there aren't any existing tests for the `explore`
command, so I didn't know where I should add new tests to cover my code
changes.
---------
Co-authored-by: paulie4 <203125+paulie4@users.noreply.github.com>
Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
# Description
Cleanups:
- Add "key_press" to event reading function names to match reality
- Move the relevant comment about why only key press events are
interesting one layer up
- Remove code duplication in handle_events
- Make `try_next` try harder (instead of bail on a boring event); I
think that was the original intention
- Remove recursion from `next` (I think that's clearer? but maybe just
what I'm used to)
# User-Facing Changes
None
# Tests + Formatting
This cleans up existing code, no new test coverage.
# Description
This PR introduces a new `Signals` struct to replace our adhoc passing
around of `ctrlc: Option<Arc<AtomicBool>>`. Doing so has a few benefits:
- We can better enforce when/where resetting or triggering an interrupt
is allowed.
- Consolidates `nu_utils::ctrl_c::was_pressed` and other ad-hoc
re-implementations into a single place: `Signals::check`.
- This allows us to add other types of signals later if we want. E.g.,
exiting or suspension.
- Similarly, we can more easily change the underlying implementation if
we need to in the future.
- Places that used to have a `ctrlc` of `None` now use
`Signals::empty()`, so we can double check these usages for correctness
in the future.
cc: @zhiburt
This is an internal refactoring for `explore`.
Previously, views inside `explore` were created with default/incorrect
configuration and then the correct configuration was passed to them
using a function called `setup()`. I believe this was because
configuration was dynamic and could change while `explore` was running.
After https://github.com/nushell/nushell/pull/10259, configuration can
no longer be changed on the fly. So we can clean this up by removing
`setup()` and passing configuration to views when they are created.
Configuration in `explore` has always been confusing to me. This PR
overhauls (and simplifies, I think) how configuration is done.
# Details
1. Configuration is now strongly typed. In `Explore::run()` we create an
`ExploreConfig` struct from the more general Nu configuration and
arguments to `explore`, then pass that struct to other parts of
`explore` that need configuration. IMO this is a lot easier to reason
about and trace than the previous approach of creating a
`HashMap<String, Value>` and then using that to make various structs
elsewhere.
2. We now inherit more configuration from the config used for regular Nu
tables
1. Border/line styling now uses the `separator` style used for regular
Nu tables, the special `explore.split_line` config point has been
retired.
2. Cell padding in tables is now controlled by `table.padding` instead
of the undocumented `column_padding_left`/`column_padding_right` config
3. The (optional, previously not enabled by default) `selected_row` and
`selected_column` configuration has been removed. We now only highlight
the selected cell. I could re-add this if people really like the feature
but I'm guessing nobody uses it.
The interface still looks the same with a default/empty config.nu:
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 🤔
## After
The database gets displayed like a record
## 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.
# Description
Duration can not be negative, and an underflow causes a panic.
This should fix#12539 as from what I can tell that bug was caused in
`nu-explore:📟:events` from subtracting durations, but I figured
this might be more widespread, and saturating to zero generally makes
sense.
I also added the relevant clippy lint to try to prevent this from
happening in the future. I can't think of a reason we would ever want to
subtract durations without checking first.
cc @fdncred
# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`
# Description
This adds a `SharedCow` type as a transparent copy-on-write pointer that
clones to unique on mutate.
As an initial test, the `Record` within `Value::Record` is shared.
There are some pretty big wins for performance. I'll post benchmark
results in a comment. The biggest winner is nested access, as that would
have cloned the records for each cell path follow before and it doesn't
have to anymore.
The reusability of the `SharedCow` type is nice and I think it could be
used to clean up the previous work I did with `Arc` in `EngineState`.
It's meant to be a mostly transparent clone-on-write that just clones on
`.to_mut()` or `.into_owned()` if there are actually multiple
references, but avoids cloning if the reference is unique.
# User-Facing Changes
- `Value::Record` field is a different type (plugin authors)
# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`
# After Submitting
- [ ] use for `EngineState`
- [ ] use for `Value::List`
Those allocations are all small and insignificant in the grand scheme of
things and the optimizer may be able to resolve some of those but better
to be nice anyways.
Primarily inspired by the new
[`clippy::assigning_clones`](https://rust-lang.github.io/rust-clippy/master/index.html#/assigning_clones)
- **Avoid reallocs with `clone_from` in `nu-parser`**
- **Avoid realloc on assignment in `Stack`**
- **Fix `clippy::assigning_clones` in `nu-cli`**
- **Reuse allocations in `nu-explore` if possible**
# Description
When implementing a `Command`, one must also import all the types
present in the function signatures for `Command`. This makes it so that
we often import the same set of types in each command implementation
file. E.g., something like this:
```rust
use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack};
use nu_protocol::{
record, Category, Example, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData,
ShellError, Signature, Span, Type, Value,
};
```
This PR adds the `nu_engine::command_prelude` module which contains the
necessary and commonly used types to implement a `Command`:
```rust
// command_prelude.rs
pub use crate::CallExt;
pub use nu_protocol::{
ast::{Call, CellPath},
engine::{Command, EngineState, Stack},
record, Category, Example, IntoInterruptiblePipelineData, IntoPipelineData, IntoSpanned,
PipelineData, Record, ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value,
};
```
This should reduce the boilerplate needed to implement a command and
also gives us a place to track the breadth of the `Command` API. I tried
to be conservative with what went into the prelude modules, since it
might be hard/annoying to remove items from the prelude in the future.
Let me know if something should be included or excluded.
# Description
Fixes some ignored clippy lints.
# User-Facing Changes
Changes some signatures and return types to `&dyn Command` instead of
`&Box<dyn Command`, but I believe this is only an internal change.
# Description
Following #11851, this PR adds one final conversion function for
`Value`. `Value::coerce_str` takes a `&Value` and converts it to a
`Cow<str>`, creating an owned `String` for types that needed converting.
Otherwise, it returns a borrowed `str` for `String` and `Binary`
`Value`s which avoids a clone/allocation. Where possible, `coerce_str`
and `coerce_into_string` should be used instead of `coerce_string`,
since `coerce_string` always allocates a new `String`.
# Description
This PR renames the conversion functions on `Value` to be more consistent.
It follows the Rust [API guidelines](https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv) for ad-hoc conversions.
The conversion functions on `Value` now come in a few forms:
- `coerce_{type}` takes a `&Value` and attempts to convert the value to
`type` (e.g., `i64` are converted to `f64`). This is the old behavior of
some of the `as_{type}` functions -- these functions have simply been
renamed to better reflect what they do.
- The new `as_{type}` functions take a `&Value` and returns an `Ok`
result only if the value is of `type` (no conversion is attempted). The
returned value will be borrowed if `type` is non-`Copy`, otherwise an
owned value is returned.
- `into_{type}` exists for non-`Copy` types, but otherwise does not
attempt conversion just like `as_type`. It takes an owned `Value` and
always returns an owned result.
- `coerce_into_{type}` has the same relationship with `coerce_{type}` as
`into_{type}` does with `as_{type}`.
- `to_{kind}_string`: conversion to different string formats (debug,
abbreviated, etc.). Only two of the old string conversion functions were
removed, the rest have been renamed only.
- `to_{type}`: other conversion functions. Currently, only `to_path`
exists. (And `to_string` through `Display`.)
This table summaries the above:
| Form | Cost | Input Ownership | Output Ownership | Converts `Value`
case/`type` |
| ---------------------------- | ----- | --------------- |
---------------- | -------- |
| `as_{type}` | Cheap | Borrowed | Borrowed/Owned | No |
| `into_{type}` | Cheap | Owned | Owned | No |
| `coerce_{type}` | Cheap | Borrowed | Borrowed/Owned | Yes |
| `coerce_into_{type}` | Cheap | Owned | Owned | Yes |
| `to_{kind}_string` | Expensive | Borrowed | Owned | Yes |
| `to_{type}` | Expensive | Borrowed | Owned | Yes |
# User-Facing Changes
Breaking API change for `Value` in `nu-protocol` which is exposed as
part of the plugin API.
# Description
Since #10841 the goal is to remove the implementation details of
`Record` outside of core operations.
To this end use Record iterators and map-like accessors in a bunch of
places. In this PR I try to collect the boring cases where I don't
expect any dramatic performance impacts or don't have doubts about the
correctness afterwards
- Use checked record construction in `nu_plugin_example`
- Use `Record::into_iter` in `columns`
- Use `Record` iterators in `headers` cmd
- Use explicit record iterators in `split-by`
- Use `Record::into_iter` in variable completions
- Use `Record::values` iterator in `into sqlite`
- Use `Record::iter_mut` for-loop in `default`
- Change `nu_engine::nonexistent_column` to use iterator
- Use `Record::columns` iter in `nu-cmd-base`
- Use `Record::get_index` in `nu-command/network/http`
- Use `Record.insert()` in `merge`
- Refactor `move` to use encapsulated record API
- Use `Record.insert()` in `explore`
- Use proper `Record` API in `explore`
- Remove defensiveness around record in `explore`
- Use encapsulated record API in more `nu-command`s
# User-Facing Changes
None intentional
# Tests + Formatting
(-)
Currently, `ctrl+z` is the command to exit `explore` no matter where you
are in the UI. IMO this is a bit unintuitive since that's usually used
to suspend a process.
After this change, `ctrl+c`, `ctrl+d`, and `ctrl+q` all work to exit
`explore`.
I think these are all shortcuts that users might try when attempting to
exit `explore`, and I think we might as well handle them all.
Removing 2 underused config options from `explore`.
`show_banner` controls whether `For help type :help"` is shown in the
message area when `explore is first launched. I don't think there's any
good reason not to show it, it's not a modal dialog or anything.
`exit_esc` controls whether to exit `explore` when `esc` is pressed and
we can't "go up" any further (or at least that's what it's supposed to
do, looking at the code I'm not so sure). IMO we don't need to make this
kind of basic interaction configurable unless there's a really good
reason.
## Context
`explore` is complicated and we want to overhaul its design. It will be
easier to make meaningful changes if `explore` is a little slimmer
first, so I'm trying to pare back unused/underused code and config as a
starting point.
I'm gonna be making more PRs like this, I'll try to keep them
small+self-contained.
related to
- https://github.com/nushell/nushell/issues/7819
# Description
this PR does not quite address
https://github.com/nushell/nushell/issues/7819 because it does not
implement configurable keybindings for `explore` but rather only adds
support for Vim-like motions *out of the box*.
# User-Facing Changes
in *view* and *cursor* modes,
- `h`, `j`, `k` and `l` give standard Qwerty-based Vim motions
- `g` and `G` go to the top and the end respectively
- `u` and `d` scroll up and down
> **Note**
> the bindings do not support the use of modifiers for now, so it's not
`c-u` and `c-d` which scroll pages but rather `u` and `d`
# Tests + Formatting
# After Submitting
# Description
This PR creates a new `Record` type to reduce duplicate code and
possibly bugs as well. (This is an edited version of #9648.)
- `Record` implements `FromIterator` and `IntoIterator` and so can be
iterated over or collected into. For example, this helps with
conversions to and from (hash)maps. (Also, no more
`cols.iter().zip(vals)`!)
- `Record` has a `push(col, val)` function to help insure that the
number of columns is equal to the number of values. I caught a few
potential bugs thanks to this (e.g. in the `ls` command).
- Finally, this PR also adds a `record!` macro that helps simplify
record creation. It is used like so:
```rust
record! {
"key1" => some_value,
"key2" => Value::string("text", span),
"key3" => Value::int(optional_int.unwrap_or(0), span),
"key4" => Value::bool(config.setting, span),
}
```
Since macros hinder formatting, etc., the right hand side values should
be relatively short and sweet like the examples above.
Where possible, prefer `record!` or `.collect()` on an iterator instead
of multiple `Record::push`s, since the first two automatically set the
record capacity and do less work overall.
# User-Facing Changes
Besides the changes in `nu-protocol` the only other breaking changes are
to `nu-table::{ExpandedTable::build_map, JustTable::kv_table}`.
This PR reverts https://github.com/nushell/nushell/pull/9391
We try not to revert PRs like this, though after discussion with the
Nushell team, we decided to revert this one.
The main reason is that Nushell, as a codebase, isn't ready for these
kinds of optimisations. It's in the part of the development cycle where
our main focus should be on improving the algorithms inside of Nushell
itself. Once we have matured our algorithms, then we can look for
opportunities to switch out technologies we're using for alternate
forms.
Much of Nushell still has lots of opportunities for tuning the codebase,
paying down technical debt, and making the codebase generally cleaner
and more robust. This should be the focus. Performance improvements
should flow out of that work.
Said another, optimisation that isn't part of tuning the codebase is
premature at this stage. We need to focus on doing the hard work of
making the engine, parser, etc better.
# User-Facing Changes
Reverts the HashMap -> ahash change.
cc @FilipAndersson245
# Description
see https://github.com/nushell/nushell/issues/9390
using `ahash` instead of the default hasher. this will not affect
compile time as we where already building `ahash`.
# 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 -A
clippy::needless_collect -A clippy::result_large_err` to check that
you're using the standard code style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- crates/nu-std/tests/run.nu` 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.
-->
Must be fixed; (though I'd test it)
Thanks for the reference [fdncred](https://github.com/fdncred).
close#9128
---------
Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
# Description
Lint: `clippy::uninlined_format_args`
More readable in most situations.
(May be slightly confusing for modifier format strings
https://doc.rust-lang.org/std/fmt/index.html#formatting-parameters)
Alternative to #7865
# User-Facing Changes
None intended
# Tests + Formatting
(Ran `cargo +stable clippy --fix --workspace -- -A clippy::all -D
clippy::uninlined_format_args` to achieve this. Depends on Rust `1.67`)
# Description
This PR bumps the required rust version to 1.66.1.
# User-Facing 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 -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.
I have changed `assert!(a == b)` calls to `assert_eq!(a, b)`, which give
better error messages. Similarly for `assert!(a != b)` and
`assert_ne!(a, b)`. Basically all instances were comparing primitives
(string slices or integers), so there is no loss of generality from
special-case macros,
I have also fixed a number of typos in comments, variable names, and a
few user-facing messages.
ref #7339 - This PR updates explore to take some of the colors from
nushell, namely the line colors and the ls_colors.
note: Not sure why this regression appeared maybe it's a feature or it's
no longer supposed to be supported?
Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
A few small tweaks to the new `explore` command:
1. Rewrote the help text a bit.
1. I think it's important to mention `:try` up front.
2. Removed the info about `:help foo` because it's currently supported
by very few subcommands
2. Make `exit_esc` default to true. I want to avoid people getting stuck
in `explore` like they get stuck in Vim
3. ~~Always show the help message ("For help type :help") on startup~~
1. The message is small+unobtrusive and I don't this is worth a
configuration item
4. Exit the information view when Escape is pressed
5. General typo+grammar cleanup
cc: @zhiburt @fdncred
# Description
Closes#6909. You can now add closures to your `color_config` themes.
Whenever a value would be printed with `table`, the closure is run with
the value piped-in. The closure must return either a {fg,bg,attr} record
or a color name (`'light_red'` etc.). This returned style is used to
colour the value.
This is entirely backwards-compatible with existing config.nu files.
Example code excerpt:
```
let my_theme = {
header: green_bold
bool: { if $in { 'light_cyan' } else { 'light_red' } }
int: purple_bold
filesize: { |e| if $e == 0b { 'gray' } else if $e < 1mb { 'purple_bold' } else { 'cyan_bold' } }
duration: purple_bold
date: { (date now) - $in | if $in > 1wk { 'cyan_bold' } else if $in > 1day { 'green_bold' } else { 'yellow_bold' } }
range: yellow_bold
string: { if $in =~ '^#\w{6}$' { $in } else { 'white' } }
nothing: white
```
Example output with this in effect:
Slightly important notes:
* Some color_config names, namely "separator", "empty" and "hints", pipe
in `null` instead of a value.
* Currently, doing anything non-trivial inside a closure has an
understandably big perf hit. I currently do not actually recommend
something like `string: { if $in =~ '^#\w{6}$' { $in } else { 'white' }
}` for serious work, mainly because of the abundance of string-type data
in the world. Nevertheless, lesser-used types like "date" and "duration"
work well with this.
* I had to do some reorganisation in order to make it possible to call
`eval_block()` that late in table rendering. I invented a new struct
called "StyleComputer" which holds the engine_state and stack of the
initial `table` command (implicit or explicit).
* StyleComputer has a `compute()` method which takes a color_config name
and a nu value, and always returns the correct Style, so you don't have
to worry about A) the color_config value was set at all, B) whether it
was set to a closure or not, or C) which default style to use in those
cases.
* Currently, errors encountered during execution of the closures are
thrown in the garbage. Any other ideas are welcome. (Nonetheless, errors
result in a huge perf hit when they are encountered. I think what should
be done is to assume something terrible happened to the user's config
and invalidate the StyleComputer for that `table` run, thus causing
subsequent output to just be Style::default().)
* More thorough tests are forthcoming - ran into some difficulty using
`nu!` to take an alternative config, and for some reason `let-env config
=` statements don't seem to work inside `nu!` pipelines(???)
* The default config.nu has not been updated to make use of this yet. Do
tell if you think I should incorporate that into this.
# User-Facing Changes
See above.
# 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 --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
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.
