Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/completion #202

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

a-moreira
Copy link

closes #147

@a-moreira
Copy link
Author

a-moreira commented Jul 6, 2023

this implementation is not ideal because the trait Completion in dialoguer expects only one completion (a Option<String>, while in our cause it would be better to expect a vector with possible completions. I'm open to other approaches in the long run, maybe using the crate inquire instead of dialoguer in the repo, which also provides Autocompletion. What do you think? @piegamesde

This implementation should be good enough though until this decision is made.

@a-moreira
Copy link
Author

btw also ran clippy in other parts of the repo, hope it's not a problem.

@a-moreira
Copy link
Author

some tests still breaking, will fix soon.

@piegamesde maybe it's not worth it to change to dialoguer, then we would need a way to expose the API you had written in a "loop" to get the input.

@piegamesde
Copy link
Member

I think dialoguer is fine for now. It only does completion and not suggestions, meaning that if there are multiple matching values no completion should be performed. This is not optimal, but okay for now (the Python implementation does it like this as well).

However, the magic wormhole library should not depend on dialoguer, that dependency should be exclusive to the CLI. Therefore you need to move the respective code out.

@a-moreira
Copy link
Author

@piegamesde I'm getting a cyclic dependency error when trying to import the CLI code into the root crate to get the wordlist mod, which is necessary in src/core.rs. Do we need to have the CLI as a separate crate in the root? Maybe we can change the project structure a little bit to have the CLI as a mod inside the crate root or another approach.

@piegamesde
Copy link
Member

CLI is separate from the rest on purpose, and if you are getting a cyclic dependency error that indicates you are doing something wrong. The library crate should never depend on the CLI or CLI code, and you trying to import CLI code in the library is what causes your error.

@a-moreira
Copy link
Author

a-moreira commented Jul 7, 2023

@piegamesde you are right. I changed to an approach of wrapping the main wordlist struct in a new type so I can implement the dialoguer traits inside the CLI, thus splitting the code for each crate according to its necessary functionalities. Also removed a couple of tests that don't make sense anymore now that it doesn't return a vector of suggestions.

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Patch coverage: 57.57% and project coverage change: -0.03 ⚠️

Comparison is base (52a12e2) 40.52% compared to head (ab35282) 40.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   40.52%   40.50%   -0.03%     
==========================================
  Files          23       23              
  Lines        3326     3343      +17     
==========================================
+ Hits         1348     1354       +6     
- Misses       1978     1989      +11     
Impacted Files Coverage Δ
cli/src/util.rs 0.00% <0.00%> (ø)
src/lib.rs 14.28% <ø> (ø)
src/transfer/cancel.rs 21.00% <0.00%> (ø)
src/transit/crypto.rs 43.26% <0.00%> (ø)
cli/src/main.rs 4.32% <50.00%> (+4.32%) ⬆️
src/core/wordlist.rs 96.55% <88.88%> (+3.07%) ⬆️
src/core.rs 74.19% <100.00%> (ø)
src/core/key.rs 98.61% <100.00%> (ø)
src/uri.rs 90.24% <100.00%> (ø)
src/util.rs 37.11% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@a-moreira
Copy link
Author

a-moreira commented Jul 13, 2023

one cargo test action seems to be failing for some reason, but the others not. Locally it runs ok.

}

#[allow(dead_code)] // TODO make this API public one day
pub fn get_completions(&self, prefix: &str) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be kept, and the wrapper for Dialoguer in the CLI should make use of it.

num_words,
words: load_pgpwords(),
}
}

pub fn vecstrings(all: &str) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is really weird. At the very least it should have a descriptive name and documentation to tell what it does, but honestly I think it probably shouldn't exist / not be public in the first place.

struct WordList(PgpWordList);

impl Default for WordList {
fn default() -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a re-implementation of load_pgpwords. Why not make use of that?

if completions.len() == 1 {
Some(completions.first().unwrap().clone())
} else {
// TODO: show vector of suggestions somehow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider that out of scope for now.

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

Successfully merging this pull request may close these issues.

Tab completion
2 participants