meejah / fowl

Forward over Wormhole: streams over magic-wormhole Dilation connections
MIT License
29 stars 2 forks source link

remove invite and accept subcommands #14

Closed balejk closed 9 months ago

balejk commented 1 year ago

While the distinction is somewhat intuitive, the underlying code is the same and thus it only leads to code duplication.

Marked as draft as I have not tested the changes to the tests (nor updated the documentation).

I believe this resolves our conversation on my very first PR.

meejah commented 9 months ago

I believe the extensive refactor in the latest release accomplishes something similar as this -- fowld itself loses the invite/accept subcommands, but the "human" version retains them (for ease-of-use).

balejk commented 9 months ago

I haven't looked at the final version of the changes yet, but I did make a mental note to then review whether they make this and the other PR obsolete and close them if that is the case. But you beat me to it :-)

Based on what you write, I think the other PR is definitely obsolete. If I have any reservations here, I will be sure to reopen the discussion, if that is OK with you.

meejah commented 9 months ago

yes, absolutely .. re-open this too if so :)

balejk commented 9 months ago

So, at the first glance, I think removing the subcommands would make more sense to me. I can't see any added value and on the contrary, they obfuscate the symmetry and if I would like to start an "invite" session with a predefined code, I would have to issue fowl accept..., which might be confusing. What do you think?

One thing that occurred to me as I write this: perhaps it is not desirable to have the to-be-allocated-code supplied manually as it might compromise its randomness and hence the session security? Then I suppose having the CLI designed like this (with the subcommands) would make sense to signal that it is not the recommended use while still making it technically possible via the command above.

meejah commented 9 months ago

Ahh, interesting.

So my take here was to make an "easy-to-use, happy-path" CLI in front of the more "expert-mode" fowld.

I can see your point about losing the "symmetry" though. (Also I would consider "choose your own code" probably gets into "expert mode" a little, but might be nice to support that too).

What's the actual proposal here, then? Something like "if you specify --code great, otherwise it allocates one"? So fowl invite becomes fowl and fowl accept becomes fowl --code 1-foo-bar approximately?

There's also fowl tui now which I made basically because I want to do the fowld stuff, but don't want to type in JSON (so it's meant to pretty directly mirror that protocol).

balejk commented 9 months ago

(Also I would consider "choose your own code" probably gets into "expert mode" a little, but might be nice to support that too).

Yes, that's a bit what I touched with the security concerns too. And like I said, technically you do support it, but in a somewhat confusing way. So if your concern is the security/expert mode, then I think that's a good argument (in the case of security, it could perhaps even be considered doing more to discourage people from making up their own possibly weak codes). Otherwise, I think I do consider the split redundant and the subcommand-less variant seems more natural to me.

What's the actual proposal here, then? Something like "if you specify --code great, otherwise it allocates one"? So fowl invite becomes fowl and fowl accept becomes fowl --code 1-foo-bar approximately?

I imagine it would work as this PR made it for the old version: if the code is supplied, then either allocate it or find the peer who has already allocated it. If no code is supplied, generate one. I would drop the --code flag, keeping just --code-length. So fowl invite becomes fowl (optionally with --code-length), fowl accept code becomes fowl code (it would probably make sense to have code conflict with --code-length, but I do think the code should be an optional positional argument, rather than a flag.

There's also fowl tui now which I made basically because I want to do the fowld stuff, but don't want to type in JSON (so it's meant to pretty directly mirror that protocol).

I haven't tried the TUI yet (nor anything else yet actually, so far I have just looked at the source a little bit) but I am looking forward to it.

meejah commented 9 months ago

Hmm, I was trying to hack this together quickly, but I'm not sure that Click supports having a "group" that also has subcommands.

i.e. fowl tui and fowl readme etc ...

balejk commented 9 months ago

Ah, I didn't realize there are other non-flag commands...

Hmm, what would you say to making those flags? E. g. fowl tui -> fowl -t, fowl readme -> fowl -i/--info (or something). The code could then perhaps apply even for the TUI (if that makes sense, I still haven't seen it), like fowl -t code would open TUI and use the code.

meejah commented 9 months ago

See the PR -- that's what you meant right?

Yeah the "readme" could be ... something like --info or --readme.

TUI could be a third entry-point (e.g. fowl-tui or similar)

Makes sense from Click's perspective: is "fowl readme" a subcommand, or "fowl" with a single argument that is "readme"?

meejah commented 9 months ago

Okay, I hung --readme off fowl and added a fowl-tui entry-point ... seems fine?