magic-wormhole / magic-wormhole.rs

Rust implementation of Magic Wormhole, with new features and enhancements
European Union Public License 1.2
645 stars 72 forks source link

Feat/completion #202

Open a-moreira opened 1 year ago

a-moreira commented 1 year ago

closes #147

a-moreira commented 1 year ago

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 commented 1 year ago

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

a-moreira commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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[bot] commented 1 year ago

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 ```diff @@ 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](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole) | Coverage Δ | | |---|---|---| | [cli/src/util.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-Y2xpL3NyYy91dGlsLnJz) | `0.00% <0.00%> (ø)` | | | [src/lib.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2xpYi5ycw==) | `14.28% <ø> (ø)` | | | [src/transfer/cancel.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL3RyYW5zZmVyL2NhbmNlbC5ycw==) | `21.00% <0.00%> (ø)` | | | [src/transit/crypto.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL3RyYW5zaXQvY3J5cHRvLnJz) | `43.26% <0.00%> (ø)` | | | [cli/src/main.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-Y2xpL3NyYy9tYWluLnJz) | `4.32% <50.00%> (+4.32%)` | :arrow_up: | | [src/core/wordlist.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUvd29yZGxpc3QucnM=) | `96.55% <88.88%> (+3.07%)` | :arrow_up: | | [src/core.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUucnM=) | `74.19% <100.00%> (ø)` | | | [src/core/key.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUva2V5LnJz) | `98.61% <100.00%> (ø)` | | | [src/uri.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL3VyaS5ycw==) | `90.24% <100.00%> (ø)` | | | [src/util.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL3V0aWwucnM=) | `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.

a-moreira commented 12 months ago

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