magic-wormhole.rs icon indicating copy to clipboard operation
magic-wormhole.rs copied to clipboard

Feat/completion

Open a-moreira opened this issue 1 year ago • 9 comments

closes #147

a-moreira avatar Jul 06 '23 17:07 a-moreira

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 avatar Jul 06 '23 17:07 a-moreira

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

a-moreira avatar Jul 06 '23 17:07 a-moreira

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.

a-moreira avatar Jul 06 '23 19:07 a-moreira

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.

piegamesde avatar Jul 07 '23 10:07 piegamesde

@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.

a-moreira avatar Jul 07 '23 12:07 a-moreira

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.

piegamesde avatar Jul 07 '23 12:07 piegamesde

@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.

a-moreira avatar Jul 07 '23 13:07 a-moreira

Codecov Report

Patch coverage: 57.57% and project coverage change: -0.03 :warning:

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%) :arrow_up:
src/core/wordlist.rs 96.55% <88.88%> (+3.07%) :arrow_up:
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%> (ø)

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

codecov[bot] avatar Jul 07 '23 13:07 codecov[bot]

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

a-moreira avatar Jul 13 '23 04:07 a-moreira