uname: Refactor into public fns for Nushell #5921
Conversation
dmatos2012
force-pushed
the
refactor_and_make_public_uname
branch
from
038a549
to
d2913ec
Compare
|
GNU testsuite comparison: |
|
not really about this PR but it made me think about testing. How can we make sure we don't regress these changes for you folks ? |
I am still making a small change, so not ready still the PR. But, what do you think about this @fdncred |
|
@sylvestre @dmatos2012 We hope to have tests in nushell that I guess will do the I think the best we can do is have our "normal" testing in this repo and once we integrate have the nushell level of testing in that repo. @dmatos2012 has done a great job getting good tests setup in the nushell repo, so I hope to continue that strategy. I'm not sure what else we can do. I wouldn't think you'd want to run nushell tests in this repo, but that's the only other thing I can think of. |
src/uu/uname/src/uname.rs
Outdated
| pub mod options { | ||
| pub static ALL: &str = "all"; | ||
| pub static KERNEL_NAME: &str = "kernel-name"; | ||
| pub static NODENAME: &str = "nodename"; | ||
| pub static KERNEL_VERSION: &str = "kernel-version"; | ||
| pub static KERNEL_RELEASE: &str = "kernel-release"; | ||
| pub static MACHINE: &str = "machine"; | ||
| pub static PROCESSOR: &str = "processor"; | ||
| pub static HARDWARE_PLATFORM: &str = "hardware-platform"; | ||
| pub static OS: &str = "operating-system"; | ||
| static ALL: &str = "all"; | ||
| static KERNEL_NAME: &str = "kernel-name"; | ||
| static NODENAME: &str = "nodename"; | ||
| static KERNEL_VERSION: &str = "kernel-version"; | ||
| static KERNEL_RELEASE: &str = "kernel-release"; | ||
| static MACHINE: &str = "machine"; | ||
| static PROCESSOR: &str = "processor"; | ||
| static HARDWARE_PLATFORM: &str = "hardware-platform"; | ||
| static OS: &str = "operating-system"; |
There was a problem hiding this comment.
I'd like to keep this for consistency with the other utilities. It's also a change that is not really relevant to nushell.
There was a problem hiding this comment.
That is still there, what I changed was mod Options {} to the same consistent way I saw in mv and cp with pub struct Options {} unless you meant something else.?
There was a problem hiding this comment.
You did removed the options module around the arguments right? That's what I meant
There was a problem hiding this comment.
ah ok sorry, the + liens in the diff tripped me up, bad brain fart , but added it back again
src/uu/uname/src/uname.rs
Outdated
| .for_each(|name| { | ||
| if let Some(name) = name { | ||
| output.push_str(name); | ||
| output.push(' '); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This looks like it could be done with .flatten. So something like:
for name in [...].into_iter().flatten() {
output.push_str(name);
output.push(' ');
}There was a problem hiding this comment.
TIL; didnt know you could flatten an option. Pretty cool, thx!
src/uu/uname/src/uname.rs
Outdated
| let kernel_name = if opts.kernel_name || opts.all || none { | ||
| Some(uname.sysname().to_string_lossy().to_string()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
These are all essentially .then:
let kernel_name = (opts.kernel_name || opts.all || none).then(|| {
uname.sysname().to_string_lossy().to_string()
});
|
I think on the testing side, we should have integration tests for the parts of the API that nushell uses. At least then we'll get a warning from our own CI if we break the API. I also just listened to a talk about |
|
Thanks for the feedback @tertsdiepraam ,I think I addressed them all :) |
|
friendly ping @tertsdiepraam @sylvestre :) |
|
Thanks! |
* Refactor to use options struct and make it public for Nushell * Return the output for use in nushell * wip:opt1 * Add UNameOutput struct instead * Apply req changes * change back to mod options * uname: add empty line & fix position of comment --------- Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
Hi, This PR aims at implementing the first iteration for `uname` using `uutils`. Couple of things: * Currently my [PR](uutils/coreutils#5921) to make the required changes is pending in `uutils` repo. * I guess the number of flags has to be investigated. Still the tests cover all of them. <!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # 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. --> # 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: - [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` to check that you're using the standard code style - [X] `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - [X] `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` 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. --> --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
Hi,
As part of the initial step for using
unamein nushell, I factored theutilto have the sort of same structure asumvorucp, and thus made the respectiveOptionsandunamefn public. Thank you for your time.