CONTRIBUTING.md for standard library #8824
Conversation
bobhy
changed the title
|
The first draft has lots of placeholders like |
bobhy
force-pushed
the
stdlib_contributing
branch
from
3b142f0
to
5e429cd
Compare
|
maybe we should just mark this as a draft and wait for #8815 to land? 😋 |
fdncred
added
the
std-library
|
i also think some parts of the |
bobhy
force-pushed
the
stdlib_contributing
branch
from
24c7ea7
to
77fd337
Compare
|
Ready for review, woo-hoo! |
There was a problem hiding this comment.
thanks a looooot for this, that's amazing ✨
mainly little tweaks, a few more important ones 😉
❗ the document is very complete, i like that, but it's quite long already => i think a little table of content with [<name>](#anchor-name) links at the top would be great to navigate this quickly 😋
crates/nu-std/README.md
Outdated
| use std log assert | ||
| . . . | ||
| log debug "Running again" | ||
| assert (2 == 1) |
There was a problem hiding this comment.
with the current module system, that would not work 👀
> use std [log assert]
Error: nu::parser::export_not_found
× Export not found.
╭─[entry #2:1:1]
1 │ use std [log assert]
· ─┬─
· ╰── could not find imports
╰────however, applying the following would fix it
-use std log assert
+use std ['log debug' assert]There was a problem hiding this comment.
I definitely should have verified the example works as documented. I'll update this.
But why does use require second argument to be an array? Doesn't use take any number of arguments (rather than exactly 2, the second of which is an array ...)?
There was a problem hiding this comment.
I definitely should have verified the example works as documented. I'll update this.
no worries, this is why humanity invented the Review 😉
But why does
userequire second argument to be an array? Doesn'tusetake any number of arguments (rather than exactly 2, the second of which is an array ...)?
i do not know the "why" but...
>_ use std 'log debug' assert
Error: nu::parser::extra_positional
× Extra positional argument.
╭─[entry #1:1:1]
1 │ use std 'log debug' assert
· ───┬──
· ╰── extra positional argument
╰────
help: Usage: use <module> (members)according to help use, it takes a members but not a ...members 😮
| > the `cargo run --` part of this command is important to ensure the version of `nushell` and the version of the library are the same. | ||
| ## :pencil2: contribute to the standard library | ||
| You're invited to contribute to the standard library! | ||
| See [CONTRIBUTING.md](./CONTRIBUTING.md) for details |
There was a problem hiding this comment.
maybe a period here for consistency with the rest of the document? 😋
| 〉use std | ||
| 〉help commands | select name usage | where name =~ "std " |
There was a problem hiding this comment.
i think this will be tideous to maintain and to scale...
for the time being, i'd leave the command which is great but remove the big table 👍
There was a problem hiding this comment.
I'll telescope the table, but want to show some sample results. If user wants to see the whole list in the current version, s/he can run the command then.
There was a problem hiding this comment.
that sounds a great compromise 👍
crates/nu-std/CONTRIBUTING.md
Outdated
| as soon as you get started and mark it as work in progress (WIP). | ||
| This will alert other contributors that you're working in this area and | ||
| advertise roughly what scope of changes you're thinking of. | ||
| See [below](the_pr) for details. |
There was a problem hiding this comment.
i think that should be [below](#the-pr) to jump to the anchor.
otherwise, when i read the document on your PR branch, it wants to bring me to the invalid https://github.com/bobhy/nushell/blob/stdlib_contributing/crates/nu-std/the_pr 😱
crates/nu-std/CONTRIBUTING.md
Outdated
| [Discord implementation-chat channel](https://discord.com/channels/601130461678272522/615962413203718156) | ||
| 1. When you get to an appropriate state of doneness, push your changes to the PR and remove the WIP flag. | ||
| 2. Team members and other contributors will then review your PR. | ||
| Respond to any review comments they raise and resolve them one way or another. |
There was a problem hiding this comment.
i you mean "solve the review threads when you, as the author of the PR, thinks you have addressed them", i'd strongly discourage that 😋
as a reviewer, i find this super confusing and tideous when someone else than me, either the author or another reviewer, closes a thread i've opened for me 😕
i've opened the thread, so i'd like to know if the changes have been properly addressed 😏
when a thread is closed by someone else, it does not appear anymore in the review feed and the reviewer would have to scroll all the resolved threads to find what has been answered, if the changes have been approved, etc, etc...
of course, that is my experience and i find that very confusing myself, but i'd really like not mentionning that opportunity 👀
=> i also think specifying in the thread what commit(s) should addressed the issue is very sweet for reviewers 😌
There was a problem hiding this comment.
My use of "resolve" is confusing, maybe "address" is closer to the mark? I mean that the owner of the PR should change the code or convince the reviewer it's fine as is.
Also, is it the norm on this project that the reviewer who opens the thread can be relied on to close it when satisfied? Github allows other participants to "resolve" the thread, and I've seen projects where the owner of the PR is the only one who's motivated to clean up the threads.
There was a problem hiding this comment.
My use of "resolve" is confusing, maybe "address" is closer to the mark? I mean that the owner of the PR should change the code or convince the reviewer it's fine as is.
oh yeah perfect!
then i'd keep "address" for this very action 😉
as "resolve" is the action GitHub refers to on the button to "close" the threads, that might be misleading?
Also, is it the norm on this project that the reviewer who opens the thread can be relied on to close it when satisfied?
i do not know if there's a norm here 🤔
maybe in the main CONTRIBUTING.md?
these are my two cents 😌
i really find that super confusing not seing my threads where i left them 😕
Note
nushelldoes not require approvals nor thread resolution to merge a PR, so we do not even really need to close threads before merging.
i see them as indications for another maintainer that wants to answer the question "can i merge this PR" => an opened thread without clear closure might indicate a non-quite-ready PR
Github allows other participants to "resolve" the thread, and I've seen projects where the owner of the PR is the only one who's motivated to clean up the threads.
imo, that looks like a pretty bad practice 🤔
i think review threads ARE the review itself.
they are the place where maintainers and reviewers discuss with the author what needs to be changed, what could be changed.
if the author closes the threads, that looks almost like a hack where they say "i wont' work on this and you won't see this unless you scroll all your closed threads" => that could make the "thread resolution" requirement branch protection rule happy even though nothing has been addressed 😮
it's like doing a rebase / force-push without a reason => that disqualifies all the previous review and commits because i do not have any guarantee as a reviewer that the rebase did not break any thing 😋
in the end i'd say three things
- there is no consensus on this, this is from my personal review experience
- i find closing my threads for me really confusing and a waste of time as i need to go and get them back...
- i think the responsibility to close a thread is on the person who opened it (or maybe a very high-permission maintainer who really knows what their doing :/)
There was a problem hiding this comment.
(not targeting your force-push here @bobhy, this was a good one 😉)
There was a problem hiding this comment.
i'm really just expressing very general review (personal?) guidelines but i can not resist 👀
i think a good review workflow is really important 😋
TL;DR: changing "resolve" to "address" in this document would be just perfect 😊
crates/nu-std/CONTRIBUTING.md
Outdated
| 1. Create / update the source and test files for a custom command `foo` as follows: | ||
| ```shell | ||
| # source file | ||
| /path/to/nushell/crates/nu-std/lib/foo.nu |
There was a problem hiding this comment.
just wondering if we should only write crates/nu-std/... instead of the generic /path/to/... ?
There was a problem hiding this comment.
I have to be consistent about paths, one way or the other....
There was a problem hiding this comment.
i might prefere crates/nu-std/... a bit
- removes a bit of noise from
/path/to/... - a bit shorter
BUT it assumes you are in the root of nushell (which is not that much of a strong assumption?)
💡 i do not have a strong preference here
crates/nu-std/CONTRIBUTING.md
Outdated
| 2. Typically, the source file will implement multiple subcommands and possibly a main command as well. | ||
| For an example of a custom command which does not modify environment variables (but may reference them), see: | ||
| ```shell | ||
| /path/to/nushell/crates/nu-std/lib/assert.nu | ||
| ``` | ||
| Note the use of `export def` to define the subcommand. | ||
|
|
||
| For an example of a custom command which *does* modify environment variables, see: | ||
| ```shell | ||
| /path/to/nushell/crates/nu-std/lib/dirs.nu | ||
| ``` | ||
| Note the use of `export def-env` to define the subcommand, | ||
| the use of `export-env {}` to initialize the environment variable and `let-env` to update it. |
There was a problem hiding this comment.
almost same remark here => that might be made somewhat smaller by directly linking to the two modules without code blocks 👍
There was a problem hiding this comment.
I wanted to highlight the "note the use of ..." outside the code block. I'll come up with some other presentation for this.
crates/nu-std/CONTRIBUTING.md
Outdated
| the use of `export-env {}` to initialize the environment variable and `let-env` to update it. | ||
|
|
||
| 1. A `foo` command will be exposed to the user as `std foo` (at a minimum). | ||
| To enable this, update file `/path/to/nushell/crates/nu-std/lib/mod.rs` and add this code: |
crates/nu-std/CONTRIBUTING.md
Outdated
| export use crates/nu-std/lib/foo.nu * # command doesn't update environment | ||
| export-env { | ||
| use crates/nu-std/lib/bar.nu * # command *does* update environment | ||
| } |
There was a problem hiding this comment.
this is actually incorrect but it's a recent change in #8815 😌
should be
export use foo * # command doesn't update environment
export-env {
use bar * # command *does* update environment
}
💡 modules are automatically loaded from crates/nu-std/lib/ as .nu files thanks to the library loader in crates/nu-std/src/lib.rs 👍
There was a problem hiding this comment.
OK, the advice is "monkey see, monkey do", but it means std is becoming less and less like a typical user-written module library.
There was a problem hiding this comment.
ehe nope it's not anymore 😋
we decided, for now at least, to embed the library in the nu binary directly => the direct consequence is that we can not rely on paths like crates/nu-std/lib/<mod>.nu because the user won't have that file at all on the system!
only specifying <mod> is all the automatic loader (in crates/nu-std/src/lib.rs) needs to know 👍
crates/nu-std/CONTRIBUTING.md
Outdated
|
|
||
| ```shell | ||
| cd /path/to/nushell | ||
| NU_LOG_LEVEL=ERROR nu "crates/nu-std/tests/run.nu" |
There was a problem hiding this comment.
here it should be cargo run -- and not plain nu 😋
as the library is embedded in the nu binary, they should be in sync together => the easiest to do that is
- to use
cargo run -- ... - to use the
toolkitmodule in the root of the project 😉
There was a problem hiding this comment.
Let me understand better. Is it the case that the code for std is not loaded from the .nu files any longer?
How then can the developer try making changes to std.nu and friends? Does s/he have to rebuild the interpreter after making a script change?
Is there any dependency on an environment variable to find any of the std .nu files?
Does $env.NU_LIB_DIRS still function to allow you to omit the full path in a use <module> for modules other than std. How does Nushell tell the difference?
I want to assume a std developer already has some familiarity with modules in general, so I think I need to be careful to point out where they're different so s/he's not misled by too much knowlege.
In any case, I think the point of your correction is that developer must be sure to run the instance of the interpreter that's in the current source tree (thus cargo run, and not run the interpreter that's somewhere in his/her $env.PATH, which might be a different version. Is that it?
There was a problem hiding this comment.
Let me understand better.
👍
Is it the case that the code for std is not loaded from the
.nufiles any longer?
it is not. i mean it's the case that the code for std is not loaded from the .nu files.
as i said in the thread just above, the nu-std code loads the modules by name just before the REPL starts or a script runs.
How then can the developer try making changes to std.nu and friends? Does s/he have to rebuild the interpreter after making a script change?
yes they should rebuild => hence the cargo run
you could have an old version of nu installed which could make nu .../tests/run.nu behave unexpectedly.
PLUS we can not rely, in the end, on the user having the files on their filesystem, because apriori they won't!
Is there any dependency on an environment variable to find any of the std
.nufiles? Does$env.NU_LIB_DIRSstill function to allow you to omit the full path in ause <module>for modules other thanstd. How does Nushell tell the difference?
the nu-std loader loads the whole library before startup and make it available to the user as std => you just have to call use std to import all the library.
Note
it's a bit like the library "being in$env.NU_LIB_DIRS", but it's really at the binary level =>nushelldoes not look up in$env.NU_LIB_DIRSforstd, it just loads it directly from the binary.
I want to assume a std developer already has some familiarity with modules in general, so I think I need to be careful to point out where they're different so s/he's not misled by too much knowlege.
absolutely!
the only "new" rule to follow here is to use the name of the modules only not their paths 😌
In any case, I think the point of your correction is that developer must be sure to run the instance of the interpreter that's in the current source tree (thus
cargo run, and not run the interpreter that's somewhere in his/her $env.PATH, which might be a different version. Is that it?
that's exactly right 👌
the worse really would be having a nu binary which does not feature the std loader / relies on path to the std modules...
that would very likely lead to unexpected behaviour or more probably just crash 😱
in the end, i did not try such a thing, but from what i wrote in the loader, that does not look right 😋
hope all of this is a bit clearer 😌
and reference CONTRIBUTING
break long lines for maintainability random wordsmithing.
> **Note** > waiting for the following to land > - #8824 to avoid conflicts, i'll add this to `CONTRIBUTING.md` once it's ready 👍 should close #8839 # Description this PR moves the `log` submodule of `std` to the top of the call stack, making it available in the rest of the library as `log`. i've added some comments around the `submodules` list in `load_standard_library` to make it clear how it should work. # User-Facing Changes `log` and `assert` are now available in the rest of `std`. # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - ⚫ `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting ``` $nothing ```
bobhy
force-pushed
the
stdlib_contributing
branch
from
b83dbef
to
da4be83
Compare
commit 8ee52b6 Author: JT <547158+jntrnr@users.noreply.github.com> Date: Wed Apr 12 05:21:52 2023 +1200 Relax the closure syntax, highlight differently (nushell#8846) This relaxes the closure syntax so that `||` is no longer required. This allows for `ls | each { $in.name }` for example. I've gone ahead and changed the syntax highlighting so that blocks and closures are distinct for now. Removes `||` requirement for closures. 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 - `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 > ``` 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. commit 46dba88 Author: JT <547158+jntrnr@users.noreply.github.com> Date: Wed Apr 12 05:21:31 2023 +1200 Revert numberlike parsing restriction (nushell#8845) This effectively reverts nushell#8635. We shipped this change with 0.78 and received many comments/issues related to this restriction feeling like a step backward. fixes: nushell#8844 (and probably other issues) Returns numbers and number-like values to being allowed to be bare words. Examples: `3*`, `1fb43`, `4,5`, and related. 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 - `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 > ``` 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. commit eb4d19f Author: federico viscomi <federicoviscomi@gmail.com> Date: Mon Apr 10 22:52:51 2023 +0100 Parser panic for signature with multibyte char for short flag nushell#8821 (nushell#8849) Hey I'm a developer and I'm still new to nushell and rust but I would like to learn more about both. This is my first PR for this project. The intent of my change is to allow to use multibyte utf-8 characters in commands short flags. commit c367876 Author: Máté FARKAS <fm@farkas-mate.hu> Date: Mon Apr 10 22:42:11 2023 +0200 stdlib test runner: implement `setup` and `teardown` commands to unit tests (nushell#8776) As in other testing frameworks, the `setup` runs before every test case, and the `teardown` after that. A context can be created in `setup`, which will be in the `$in` variable in the test cases, and in the `teardown`. The `teardown` is called regardless of the test is passed, skipped, or failed. For example: ```nushell use std.nu * export def setup [] { log debug "Setup is running" {msg: "This is the context"} } export def teardown [] { log debug $"Teardown is running. Context: ($in)" } export def test_assert_pass [] { log debug $"Assert is running. Context: ($in)" } export def test_assert_skip [] { log debug $"Assert is running. Context: ($in)" assert skip } export def test_assert_fail_skipped_by_default [] { log debug $"Assert is running. Context: ($in)" assert false } ```  I'll update the documentation. --------- Co-authored-by: Mate Farkas <Mate.Farkas@oneidentity.com> commit 9bb2c8f Author: Justin Ma <hustcer@outlook.com> Date: Tue Apr 11 04:10:41 2023 +0800 Upgrade to nu v0.78 for binaries release workflow (nushell#8840) Upgrade to nu v0.78 for binary packages release workflow Test Release: https://github.com/hustcer/nu-release/releases/tag/v0.78.2 Release workflow running output: https://github.com/hustcer/nu-release/actions/runs/4656319252/jobs/8239828202 _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ 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 - `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 > ``` 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. commit 74dcac3 Author: nibon7 <nibon7@163.com> Date: Tue Apr 11 02:52:42 2023 +0800 Fix process_range on 32-bit platforms (nushell#8842) This PR fixes `commands::str_::substrings_the_input_and_treats_end_index_as_length_if_blank_end_index_given` testcase on 32-bit platform. ``` failures: ---- commands::str_::substrings_the_input_and_treats_end_index_as_length_if_blank_end_index_given stdout ---- === stderr thread 'commands::str_::substrings_the_input_and_treats_end_index_as_length_if_blank_end_index_given' panicked at 'assertion failed: `(left == right)` left: `"arepa"`, right: `"arepas"`', crates/nu-command/tests/commands/str_/mod.rs:363:9 failures: commands::str_::substrings_the_input_and_treats_end_index_as_length_if_blank_end_index_given test result: FAILED. 1072 passed; 1 failed; 23 ignored; 0 measured; 0 filtered out; finished in 2.98s error: test failed, to rerun pass `-p nu-command --test main` ``` https://gitlab.alpinelinux.org/nibon7/aports/-/jobs/1005935#L3864 https://gitlab.alpinelinux.org/nibon7/aports/-/jobs/1005931#L3867 N/A 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 - `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 > ``` 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. commit 6d51e34 Author: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Mon Apr 10 20:32:33 2023 +0200 stdlib: make helper modules available in std (nushell#8841) > **Note** > waiting for the following to land > - nushell#8824 to avoid conflicts, i'll add this to `CONTRIBUTING.md` once it's ready 👍 should close nushell#8839 this PR moves the `log` submodule of `std` to the top of the call stack, making it available in the rest of the library as `log`. i've added some comments around the `submodules` list in `load_standard_library` to make it clear how it should work. `log` and `assert` are now available in the rest of `std`. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - ⚫ `toolkit test` - 🟢 `toolkit test stdlib` ``` $nothing ``` commit de76c7a Author: WindSoilder <WindSoilder@outlook.com> Date: Tue Apr 11 01:23:22 2023 +0800 Remove autoprinting of `for` loop (nushell#8843) It's an addition to nushell#8618 And I think it's good to keep the same behavior when we use for loop for list. 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 - `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 > ``` 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. commit 0e23400 Author: Stefan Holderbach <sholderbach@users.noreply.github.com> Date: Mon Apr 10 03:42:43 2023 +0200 Follow up nushell#8758 with a style fix (nushell#8822) The old comment around the question mark operator doesn't make sense to me based on the closure signature. The `match` expressions were thus superfluous. None No change commit d0a83fe Author: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Mon Apr 10 00:59:57 2023 +0200 stdlib: add completion to `help` commands (nushell#8799) > **Note** > waiting for > - nushell#8770 > - nushell#8815 this PR adds a very basic completion support to the `help` implementations of the `std`. completion on all the `std help ...` commands, based on the names in `$nu.scope.` - ⚫ `toolkit fmt` - ⚫ `toolkit clippy` - ⚫ `toolkit test` - ⚫ `toolkit test stdlib` ``` $nothing ``` commit 57510f2 Author: Stefan Holderbach <sholderbach@users.noreply.github.com> Date: Mon Apr 10 00:56:47 2023 +0200 Move CLI related commands to `nu-cli` (nushell#8832) Part of the larger cratification effort. Moves all `reedline` or shell line editor specific commands to `nu-cli`. - `commandline` - This shouldn't have moved there. Doesn't directly depend on reedline but assumes parts in the engine state that are specific to the use of reedline or a REPL - `keybindings` and subcommands - `keybindings default` - `keybindings list` - `keybindings listen` - very `reedline` specific - `history` - needs `reedline` - `history session` Instead of having a separate `create_default_context()` that calls `nu-command`'s `create_default_context()`, I added a `add_cli_context()` that updates an `EngineState` None `cargo build --timings` from a `cargo clean --profile dev` main: 64 secs this: 59 secs branch | total| codegen | fraction ---|---|---|--- main | 14.0s | 6.2s | (44%) this | 12.5s | 5.5s | (44%) `nu-cli` depends on `nu-command` at the moment. Thus it is built during the code-gen phase of `nu-command` (on 16 virtual cores) I removed the `test_example()` facilities for now as we had not run any of the commands in an `Example` test and importing the right context for those tests seemed more of a hassle than the duplicated `test_examples()` implementations in `nu-cmd-lang` and `nu-command` commit 58b96fd Author: JT <547158+jntrnr@users.noreply.github.com> Date: Mon Apr 10 10:55:29 2023 +1200 Add option to not load std-lib. Default tests to not use std-lib (nushell#8833) this adds a `--no-std-lib` flag. Moves `nu!` to use the `--no-std-lib`. Adds a new `nu_with_std!` macro for future tests that need the std-lib. _(Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes.)_ _(Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience.)_ _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ 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 - `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 > ``` 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. commit 9e3d6c3 Author: JT <547158+jntrnr@users.noreply.github.com> Date: Mon Apr 10 08:55:47 2023 +1200 Only add the std lib files once (nushell#8830) We were seeing duplicate entries for the std lib files, and this PR addresses that. Each file should now only be added once. Note: they are still parsed twice because it's hard to recover the module from the output of `parse` but a bit of clever hacking in a future PR might be able to do that. _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ 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 - `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 > ``` 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. commit 4a955d7 Author: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Sun Apr 9 19:00:20 2023 +0200 stdlib: refactor into a multi-module library (nushell#8815) commit 637283f Author: mike <98623181+1Kinoti@users.noreply.github.com> Date: Sat Apr 8 23:04:57 2023 +0300 allow register to accept a const argument (nushell#8758) this pr allows `register` to be used with const variables ```nu const math_plugin = "~/.config/nushell/plugins/nu_plugin_math" register $math_plugin ``` should close nushell#8208, previous work nushell#8435 commit 5afbfb5 Author: Maria José Solano <majosolano99@gmail.com> Date: Sat Apr 8 11:52:37 2023 -0700 Use `pretty_assertions` in the root crate (nushell#8818) This PR is just a minor development improvement. While working on another feature, I noticed that the root crate lists the super useful `pretty_assertions` in the root crate but doesn't use it in most tests. With this change `pretty_assertions::assert_eq!` is used instead of `core::assert_eq!` for better diffs when debugging the tests. I thought of adding the dependency to other crates but I decided not to since I didn't want a huge disruptive PR :) commit d128c0e Author: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Sat Apr 8 14:35:16 2023 +0200 stdlib: use the loaded library in tests and update `README` (nushell#8811) Should close nushell#8809. this PR uses the automatically loaded library from the tests by replacing `use std.nu ...` with `use std ...`. the `README` has been updated by - removing the very deprencated "concrete examples" - fixing the `use std` and the "run the tests" sections the `README` can be previewed [here](https://github.com/amtoine/nushell/blob/refactor/stdlib/use-std-in-tests-and-update-readme/crates/nu-std/README.md) 👍 ``` $nothing ``` - 🟢 `toolkit test stdlib` ``` $nothing ``` commit 60e6ea5 Author: Michael Angerman <1809991+stormasm@users.noreply.github.com> Date: Sat Apr 8 04:53:43 2023 -0700 remove nu_cli crate dependency from nu_std (nushell#8807) now nu_std only depends on nu_parser, nu_protocol and miette and removes the nu_cli dependency this enables developers moving forward to come along and implement their own CLI's without having to pull in a redundant nu-cli which will not be needed for them. I did this by moving report_error into nu_protocol which nu_std already has a dependency on anyway.... - `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 - `cargo run -- crates/nu-std/tests.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 > ``` commit 4156077 Author: Antoine Stevan <44101798+amtoine@users.noreply.github.com> Date: Sat Apr 8 13:19:09 2023 +0200 remove the "MODULE NOT FOUND" print from the parser (nushell#8816) related to nushell#8765. should close nushell#8812. this PR simply removes the `prinln("MODULE NOT FOUND")` from the parser. no more `MODULE NOT FOUND` while typing a `use` command
|
This PR isn't an "emergency" but I do think we need to circle back at some point and finish it. The force-push kind of set us back. |
and reference CONTRIBUTING
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8824 +/- ##
==========================================
+ Coverage 68.50% 68.51% +0.01%
==========================================
Files 635 636 +1
Lines 101520 101676 +156
==========================================
+ Hits 69548 69666 +118
- Misses 31972 32010 +38
|
There was a problem hiding this comment.
a few typos and broken links 👍
Note
i think themdstyle will have to be reworked at some point (lots of trailing whitespaces and the format looks a bit odd to me) BUT the content ofCONTRIBUTING.mdis ok, so i'd like to land that PR and see all this in some later "cleanup" phase 😌
|
|
||
| 1. Verify the team thinks your idea is potentially relevant and useful, as above. | ||
| 1. If it's more than a simple bug fix, open a placeholder PR | ||
| as soon as you get started and [set it to draft status][github_draft_pr]. |
There was a problem hiding this comment.
should be
-as soon as you get started and [set it to draft status][github_draft_pr].
+as soon as you get started and [set it to draft status](github_draft_pr).and, at the bottom of the document, shouldn't you remove the parentheses?
-[github_draft_pr]:(https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request)
+[github_draft_pr]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request| as soon as you get started and [set it to draft status][github_draft_pr]. | ||
| This will alert other contributors that you're working in this area and let you | ||
| advertise roughly what scope of changes you're thinking of. | ||
| See [below](#the_pr) for details. |
There was a problem hiding this comment.
-See [below](#the_pr) for details.
+See [below](#the-pr) for details.| Assuming you've already validated the need with other Nushell contributors, you're focusing on design and implementation | ||
| at this point. Share your thinking all along the way! | ||
|
|
||
| You can open a [draft][github_draft_pr] pull request based on a token code change, and use the PR comments to outline your design and user interface. You'll get feedback from |
crates/nu-std/README.md
Outdated
|
|
||
| ## :pencil2: contribute to the standard library | ||
| You're invited to contribute to the standard library! | ||
| See [CONTRIBUTING.md](https://github.com/nushell/nushell/blob/main/crates/nu-std/CONTRIBUTING.md) for details. |
There was a problem hiding this comment.
-See [CONTRIBUTING.md](https://github.com/nushell/nushell/blob/main/crates/nu-std/CONTRIBUTING.md) for details.
+See [CONTRIBUTING.md](CONTRIBUTING.md) for details.should work.
There was a problem hiding this comment.
Actually, no. Your proposed change won't work in local repo, only online, thanks to github magic. I think the risk that the online target will disappear in the future is low.
There was a problem hiding this comment.
oooh that is for local repos 😮 👍
There was a problem hiding this comment.
Hmm, do we want the link to open, or do we want the path to be obvious?
There was a problem hiding this comment.
the absolute HTTP path is good 👍
| Assuming you've already validated the need with other Nushell contributors, you're focusing on design and implementation | ||
| at this point. Share your thinking all along the way! | ||
|
|
||
| You can open a [draft][github_draft_pr] pull request based on a token code change, and use the PR comments to outline your design and user interface. You'll get feedback from |
There was a problem hiding this comment.
also i'm not sure i understand what you mean with "open a draft pull request based on a token code change" 😮
There was a problem hiding this comment.
"token" means a trivial change, a placeholder. The idea is open a PR and mark it as draft as soon as you get started, before you have any real changes to commit. This, so you can draft up a "functional spec" in the commends of the PR as you proceed.
There was a problem hiding this comment.
oh i see, i just did not know that way of saying this 😌
crates/nu-std/CONTRIBUTING.md
Outdated
|
|
||
|
|
||
| 2. A `foo` command will be exposed to the user as `std foo` (at a minimum). | ||
| To enable this, update file `./crates/nu-std/lib/mod.rs` and add this code: |
There was a problem hiding this comment.
-To enable this, update file `./crates/nu-std/lib/mod.rs` and add this code:
+To enable this, update file `./crates/nu-std/lib/mod.nu` and add this code:
crates/nu-std/CONTRIBUTING.md
Outdated
| 1. Some commands from the standard library are also hoisted into the top level, non-prefixed namespace, for example, the `help` command. You can do this for a `foo` command: | ||
| * modify `./crates/nu-std/src/lib.rs`, | ||
| * find the initialization of the "prelude" at line 70 or thereabouts | ||
| * add `(foo, foo)` |
There was a problem hiding this comment.
- * add `(foo, foo)`
+ * add `("foo", "foo")` would be more accurate 😋
There was a problem hiding this comment.
We should describe what the 2 elements are actually for. I tried some experiments and documented
my best guess (and added comments to lib.rs, which should also be reviewed.)
crates/nu-std/CONTRIBUTING.md
Outdated
| 1. Ensure your custom command provides useful help. | ||
| This is done with comments before the `def` for the custom command. | ||
| 1. Use `error make` to report can't-proceed errors to user, not `log error`. | ||
| 2. Use `log info` to provide a verbose progress messages that the user can optionally enable for troubleshooting. |
There was a problem hiding this comment.
-2. Use `log info` to provide a verbose progress messages that the user can optionally enable for troubleshooting.
+2. Use `log info` to provide verbose progress messages that the user can optionally enable for troubleshooting. right?
crates/nu-std/CONTRIBUTING.md
Outdated
| ```shell | ||
| > NU_LOG_LEVEL=ERROR cargo run -- crates/nu-std/tests/run.nu | ||
| ``` | ||
| Change 'ERROR' to 'INFO' or 'DEBUG' for increasing verbousity. |
There was a problem hiding this comment.
i'd apply
- Change 'ERROR' to 'INFO' or 'DEBUG' for increasing verbousity.
+ Change 'ERROR' to `INFO` or `DEBUG` for increased verbosity.There was a problem hiding this comment.
Doesn't press my "bad grammar" button, but OK...
Don't clean the trailing whitespace up too aggressively! I use 2 blanks at the end of the line to force MD to start a new rendered line; I use that a lot in lists. |
and reference CONTRIBUTING
yep, of course, do not worry 😌 |
…ell#8337 (nushell#8805) # Description Pick up [enhancement in `dtparse`](bspeice/dtparse#41), to preserve nanoseconds in string-to-date conversions. # User-Facing Changes An especially eagle-eyed user might notice that prior versions of nushell were losing nanosecond precision when converting string format date/time to datetime. Before: ```nushell 〉'2023-03-02T01:02:03.987654321' | into datetime | date to-record ╭────────────┬───────────╮ │ year │ 2023 │ │ month │ 3 │ │ day │ 2 │ │ hour │ 1 │ │ minute │ 2 │ │ second │ 3 │ │ nanosecond │ 987654000 │ │ timezone │ -04:00 │ ╰────────────┴───────────╯ ``` Now, it just works(tm) ```nushell 〉'2023-03-02T01:02:03.987654321' | into datetime | date to-record ╭────────────┬───────────╮ │ year │ 2023 │ │ month │ 3 │ │ day │ 2 │ │ hour │ 1 │ │ minute │ 2 │ │ second │ 3 │ │ nanosecond │ 987654321 │ │ timezone │ -04:00 │ ╰────────────┴───────────╯ ``` # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - [x] `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - [x] `cargo test --workspace` to check that all tests pass Some (unrelated) tests did fail [^err] - [x] `cargo run -- crates/nu-std/tests.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. # (Unrelated) test failures noted [^err]: Several tests are failing, unrelated to this PR ``` failures: ---- modules::module_import_env_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; use main.nu foo; foo · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:316:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- modules::module_import_env_2 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; $env.FOO · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_2' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:341:9 ---- overlays::overlay_use_do_cd_file_relative stdout ---- === stderr thread 'overlays::overlay_use_do_cd_file_relative' panicked at 'assertion failed: `(left == right)` left: `"nushell"`, right: `"test1"`', tests/overlays/mod.rs:919:9 ---- overlays::overlay_use_dont_cd_overlay stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env test1/test2/spam.nu; $env.PWD | path basename · ─────────┬───────── · ╰── file not found ╰──── thread 'overlays::overlay_use_dont_cd_overlay' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"overlay_use_dont_cd_overlay"`', tests/overlays/mod.rs:946:9 ---- parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol.nu; $env.FOO · ─────┬──── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"good"`', tests/parsing/mod.rs:177:9 ---- parsing::parse_file_relative_to_parsed_file_simple stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol/lol.nu; $env.LOL · ───────┬────── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_simple' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"lol"`', tests/parsing/mod.rs:99:9 ---- shell::const_nu_lib_dirs_relative stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[main.nu:1:1] 1 │ const NU_LIB_DIRS = [ 'scripts' ] 2 │ source-env foo.nu · ───┬── thread 'shell::const_nu_lib_dirs_relative' panicked at 'assertion failed: outcome.err.is_empty()', tests/shell/mod.rs:166:9 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"AndrásWithKitKat"`', tests/shell/pipeline/commands/external.rs:198:13 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"jt_likes_cake.txt"`', tests/shell/pipeline/commands/external.rs:173:13 failures: modules::module_import_env_1 modules::module_import_env_2 overlays::overlay_use_do_cd_file_relative overlays::overlay_use_dont_cd_overlay parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 parsing::parse_file_relative_to_parsed_file_simple shell::const_nu_lib_dirs_relative shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings test result: FAILED. 386 passed; 9 failed; 20 ignored; 0 measured; 0 filtered out; finished in 98.06s error: test failed, to rerun pass `-p nu --test main` failures: ---- modules::module_import_env_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; use main.nu foo; foo · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:316:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- modules::module_import_env_2 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; $env.FOO · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_2' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:341:9 ---- overlays::overlay_use_do_cd_file_relative stdout ---- === stderr thread 'overlays::overlay_use_do_cd_file_relative' panicked at 'assertion failed: `(left == right)` left: `"nushell"`, right: `"test1"`', tests/overlays/mod.rs:919:9 ---- overlays::overlay_use_dont_cd_overlay stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env test1/test2/spam.nu; $env.PWD | path basename · ─────────┬───────── · ╰── file not found ╰──── thread 'overlays::overlay_use_dont_cd_overlay' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"overlay_use_dont_cd_overlay"`', tests/overlays/mod.rs:946:9 ---- parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol.nu; $env.FOO · ─────┬──── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"good"`', tests/parsing/mod.rs:177:9 ---- parsing::parse_file_relative_to_parsed_file_simple stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol/lol.nu; $env.LOL · ───────┬────── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_simple' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"lol"`', tests/parsing/mod.rs:99:9 ---- shell::const_nu_lib_dirs_relative stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[main.nu:1:1] 1 │ const NU_LIB_DIRS = [ 'scripts' ] 2 │ source-env foo.nu · ───┬── thread 'shell::const_nu_lib_dirs_relative' panicked at 'assertion failed: outcome.err.is_empty()', tests/shell/mod.rs:166:9 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"AndrásWithKitKat"`', tests/shell/pipeline/commands/external.rs:198:13 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"jt_likes_cake.txt"`', tests/shell/pipeline/commands/external.rs:173:13 failures: modules::module_import_env_1 modules::module_import_env_2 overlays::overlay_use_do_cd_file_relative overlays::overlay_use_dont_cd_overlay parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 parsing::parse_file_relative_to_parsed_file_simple shell::const_nu_lib_dirs_relative shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings test result: FAILED. 386 passed; 9 failed; 20 ignored; 0 measured; 0 filtered out; finished in 98.06s error: test failed, to rerun pass `-p nu --test main` failures: ---- modules::module_import_env_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; use main.nu foo; foo · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:316:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- modules::module_import_env_2 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env main.nu; $env.FOO · ───┬─── · ╰── file not found ╰──── thread 'modules::module_import_env_2' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"foo"`', tests/modules/mod.rs:341:9 ---- overlays::overlay_use_do_cd_file_relative stdout ---- === stderr thread 'overlays::overlay_use_do_cd_file_relative' panicked at 'assertion failed: `(left == right)` left: `"nushell"`, right: `"test1"`', tests/overlays/mod.rs:919:9 ---- overlays::overlay_use_dont_cd_overlay stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env test1/test2/spam.nu; $env.PWD | path basename · ─────────┬───────── · ╰── file not found ╰──── thread 'overlays::overlay_use_dont_cd_overlay' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"overlay_use_dont_cd_overlay"`', tests/overlays/mod.rs:946:9 ---- parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol.nu; $env.FOO · ─────┬──── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"good"`', tests/parsing/mod.rs:177:9 ---- parsing::parse_file_relative_to_parsed_file_simple stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[source:1:1] 1 │ source-env lol/lol/lol.nu; $env.LOL · ───────┬────── · ╰── file not found ╰──── thread 'parsing::parse_file_relative_to_parsed_file_simple' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"lol"`', tests/parsing/mod.rs:99:9 ---- shell::const_nu_lib_dirs_relative stdout ---- === stderr Error: nu::shell::file_not_found × File not found ╭─[main.nu:1:1] 1 │ const NU_LIB_DIRS = [ 'scripts' ] 2 │ source-env foo.nu · ───┬── thread 'shell::const_nu_lib_dirs_relative' panicked at 'assertion failed: outcome.err.is_empty()', tests/shell/mod.rs:166:9 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"AndrásWithKitKat"`', tests/shell/pipeline/commands/external.rs:198:13 ---- shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings stdout ---- === stderr thread 'shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings' panicked at 'assertion failed: `(left == right)` left: `""`, right: `"jt_likes_cake.txt"`', tests/shell/pipeline/commands/external.rs:173:13 failures: modules::module_import_env_1 modules::module_import_env_2 overlays::overlay_use_do_cd_file_relative overlays::overlay_use_dont_cd_overlay parsing::parse_file_relative_to_parsed_file_dont_use_cwd_1 parsing::parse_file_relative_to_parsed_file_simple shell::const_nu_lib_dirs_relative shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_lines shell::pipeline::commands::external::it_evaluation::takes_rows_of_nu_value_strings test result: FAILED. 386 passed; 9 failed; 20 ignored; 0 measured; 0 filtered out; finished in 98.06s error: test failed, to rerun pass `-p nu --test main` ```
# Description Follow up to nushell#8849 Work for nushell#8821 Should unblock work on nushell#8808 Missing is a restriction to printables or identifiers according to [UAX31](https://www.unicode.org/reports/tr31/) # User-Facing Changes Non ASCII characters should be allowed in shortflags. # Tests + Formatting Want to add some tests for the error reporting cases
# Description this pr changes some `if-let`s to `let-else`s # User-Facing Changes none
# Description This PR adds an `items` command which allows the user to iterate over both `columns` and `values` of a `Record<>` type at the same time.  # User-Facing Changes No breaking changes, only a new `items` command. # Formatting - `cargo fmt --all -- --check` 👌 - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` 👌 - `cargo test --workspace` 👌
# Description This pr is a companion to nushell/reedline#560 Fortunally, we don't need to change too much nushell code. ## Additional note about lscolor dependency sharkdp/lscolors#58 lscolor is using 0.26 for now
# Description Fixes nushell#8878 Add continue command on each and added new tests too .
# Description _Fixes #5923_ Currently `nushell` doesn't allow short flag batches to contain arguments, despite this being a common pattern in commands like `git commit -am 'My commit message'`. This PR relaxes this so that the last flag in the batch can take an argument. # User-Facing Changes - `nu::parser::short_flag_arg_cant_take_arg` has been replaced by `nu::parser::only_last_flag_in_batch_can_take_arg` and is displayed when a flag other then the last in a short flag batch takes an argument. # Tests + Formatting - Both [`test_parser.rs`](https://github.com/nushell/nushell/blob/48af0ebc3c9f0c7b0ef134078fe1a0e47982059e/crates/nu-parser/tests/test_parser.rs#L640-L704) and [`test_known_external.rs`](https://github.com/nushell/nushell/blob/48af0ebc3c9f0c7b0ef134078fe1a0e47982059e/src/tests/test_known_external.rs#L42-L61) have been updated to test the new allowed and disallowed scenarios. --------- Co-authored-by: sholderbach <sholderbach@users.noreply.github.com>
…l#8610) # Description Make `$in` takes cell path in `update` command The reason behind the change: https://discord.com/channels/601130461678272522/615329862395101194/1088405671080370196 > when i use update on some cell path, it's almost always because i want to start with its previous value and change it. cc @amtoine # User-Facing Changes ## Before ``` open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| $in.metadata.binstall.pkg-fmt | str replace "g" "FOO"} ``` ## After ``` open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|| str replace "g" "FOO"} ``` If use want to access original raw, it can be accessed by parameters in closure: ``` open Cargo.toml | get package | update metadata.binstall.pkg-fmt {|$it| $it.metadata.binstall.pkg-fmt | str replace "g" "FOO"} ``` For this reason, I don't think we need to add a flag like `--whole` # 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 > **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.
and reference CONTRIBUTING
and reference CONTRIBUTING
break long lines for maintainability random wordsmithing.
and reference CONTRIBUTING
|
I innocently pulled latest changes from main and rebased my 3 file changes on top of that int order to keep the PR fresh and mergable. But now I see that the PR shows 35 files modified. The changes seem to be recent merges into main, but I've completely lost confidence in the integrity of this PR! I'm going to close it and open a new one with just my intended changes; let's see whether the new one can get through review a bit more expeditiously... |
|
This PR has gone off the rails and has been deprecated in favor of #8894, q.v. |
# Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Replaces #8824, which was languishing in review limbo and becoming increasingly difficult to keep current with upstream changes. In addition to all the edits, this PR includes updated documentation for running unit tests via `std run-tests`. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> A CONTRIBUTING.md documenting guidelines and getting started info for potential stdlib contributors. # 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 - `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. -->
Description
per #8810, a CONTRIBUTING.md for working on Nushell standard library.
(Description of your pull request goes here. Provide examples and/or screenshots if your changes affect the user experience.)
User-Facing Changes
Just a text file in the source tree. Nothing forces the developer to notice it, let alone abide by it.
Tests + Formatting
nil
After Submitting
nil