Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Upstreaming? #1

Closed
epage opened this issue May 20, 2023 · 8 comments · Fixed by #2
Closed

Upstreaming? #1

epage opened this issue May 20, 2023 · 8 comments · Fixed by #2

Comments

@epage
Copy link

epage commented May 20, 2023

People have expressed interest in nushell support (clap-rs/clap#2778). Would you be interested in moving development into the clap repo? I would prefer for re-licensing to be the dual license of the rest of the clap crates and for publish access to be shared with the group.

@nibon7
Copy link
Owner

nibon7 commented May 20, 2023

Sure, sounds great 😆

@epage
Copy link
Author

epage commented May 20, 2023

To move forward

  • Officially mark the license as MIT OR Apache-2.0 (looks like you are the sole contributor, so relicensing should be easy)
  • Add github:clap-rs:Admins and github:rust-cli:Maintainers as additional owners so it can be published along with the rest of the clap crates

@nibon7
Copy link
Owner

nibon7 commented May 20, 2023

  • Add github:clap-rs:Admins and github:rust-cli:Maintainers as additional owners so it can be published along with the rest of the clap crates

Failed to add github:clap-rs:Admins or github:rust-cli:Maintainers with cargo owner --add or crates.io web page. I'm sure crates.io is listed in the github Authorized OAuth Apps tab.

@epage
Copy link
Author

epage commented May 22, 2023

Not ideal but if you want, you can add me (https://crates.io/users/epage) as an owner and I can debug it

@nibon7 nibon7 closed this as completed in #2 May 23, 2023
@nibon7
Copy link
Owner

nibon7 commented May 23, 2023

Okay, I've sent the invitation. Please keep an eye out for it. :)

@epage
Copy link
Author

epage commented May 23, 2023

Thanks!

See clap-rs/clap#4935

@epage
Copy link
Author

epage commented May 23, 2023

Well, thats annoying. nushell requires 1.65 and clap's MSRV is 1.64 and officially I shouldn't bump it for a couple of months (just release 4.3, supposed to wait for 4.4). Guess it bit me saying "meh, we don't need the higher version, maybe I should skip it".

I might just in the short-term remove the tests so its at the same level of testing as everything else (but I love that there is a test harness for completions, more shells need that!)

@nibon7
Copy link
Owner

nibon7 commented May 24, 2023

I guess we can pass the completion test with rust 1.64.0 and nushell 0.78.0, just need a few small tweaks. @epage

diff --git a/clap_complete_nushell/Cargo.toml b/clap_complete_nushell/Cargo.toml
index 22622634..fb934b43 100644
--- a/clap_complete_nushell/Cargo.toml
+++ b/clap_complete_nushell/Cargo.toml
@@ -37,9 +37,9 @@ clap_complete = { path = "../clap_complete", version = "4.0.0" }
 [dev-dependencies]
 snapbox = { version = "0.4.10", features = ["diff"] }
 clap = { path = "../", version = "4.0.0", default-features = false, features = ["std", "help"] }
-nu-cli = "0.80.0"
-nu-command = "0.80.0"
-nu-parser = "0.80.0"
-nu-protocol = "0.80.0"
-nu-test-support = "0.80.0"
-reedline = "0.19.1"
+nu-cli = "0.78.0"
+nu-command = "0.78.0"
+nu-parser = "0.78.0"
+nu-protocol = "0.78.0"
+nu-test-support = "0.78.0"
+reedline = "0.18.0"
diff --git a/clap_complete_nushell/tests/completion.rs b/clap_complete_nushell/tests/completion.rs
index 730cd21c..9801b761 100644
--- a/clap_complete_nushell/tests/completion.rs
+++ b/clap_complete_nushell/tests/completion.rs
@@ -96,8 +96,8 @@ fn external_completion(file_name: &str) -> NuCompleter {
 
     let (_, delta) = {
         let mut working_set = StateWorkingSet::new(&engine_state);
-        let block = parse(&mut working_set, None, &buf, false);
-        assert!(working_set.parse_errors.is_empty());
+        let (block, err) = parse(&mut working_set, None, &buf, false, &[]);
+        assert!(err.is_none());
 
         (block, working_set.render())
     };

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants