feat: replace chat-cli /connect with /dm and /new - #206
Conversation
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
All these changes make sense to me. No blockers but a few notes.
Note: For reviews I tend to use this feedback ladder so there is no ambiguity about severity or importance -https://www.netlify.com/blog/2020/03/05/feedback-ladders-how-we-encode-code-reviews-at-netlify/
| /// Which kind of MLS conversation this is. `Dm` is a DirectV1 1:1 — no members | ||
| /// can be added; `Group` is an addable GroupV2 conversation. |
There was a problem hiding this comment.
[Dust] The Actual ConversationType being used will change overtime. Listing them explicitly will result in the docs becoming out of date quickly. Consider removing.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub enum ChatKind { | ||
| Dm, | ||
| Group, | ||
| } |
There was a problem hiding this comment.
[!] There was a previous "ConvoClass" structure but I'm realizing it never landed in generic_client. We should eventually think about exposing this information so it is available for all clients.
There was a problem hiding this comment.
what do you recommend for now? I think it would be good to do it now, I can do it in a separate small PR, let me know which approach would be ok, in this PR or another tiny PRs?
There was a problem hiding this comment.
Given this code has no impact on developers I'm not picky about order.
I think it would be good to do it now
I'll defer to you, and your priorities, and workflow.
let me know which approach would be ok
In this particular case I would put it as a separate PR. It's something we want to be slightly more intentional about, and keeps the domains separate. Specifically with a dedicated PR description detailing motivations etc.
| self.add_system_message("/connect <address> - Connect using an address"); | ||
| self.add_system_message("/dm <address> - Start a direct (1:1) chat"); |
There was a problem hiding this comment.
[Sand] This PR is tightly coupled with #205 . Short PR's are great, however its hard to see how these changes will all land together. Particularly with some unrelated lines being deleted in this PR.
In the future I don't have any issue reviewing tightly coupled items together in a single PR. Especially if the commits are clean.
There was a problem hiding this comment.
This was a rebase issue, fixed now.
| // First token is the (optional) group name; any remaining tokens | ||
| // are addresses to invite at creation. `/new` alone makes an empty | ||
| // group. |
There was a problem hiding this comment.
[?] Is there a functional reason to not force developers to create groups with a defined name?
There was a problem hiding this comment.
No, it was solo based on it's a cli to make it easier to use. Will make it mandatory.
|
|
||
| **Direct message (1:1):** | ||
|
|
||
| 1. Alice types `/account` and sends Bob her address. |
There was a problem hiding this comment.
[!] There is a proposal to remove Alice and Bob from chat level documentation : https://github.com/logos-co/logos-lips/blob/master/docs/messaging/informational/raw/chat-cast.md -- Feedback welcome
130a2d0 to
06d098f
Compare
# Conflicts: # bin/chat-cli/README.md # bin/chat-cli/src/app.rs
… into mch/libchat-refactoring-connect
2 participants
No description provided.