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

Convert ChatMessage to enum #87

Merged
merged 8 commits into from
Aug 15, 2024
Merged

Convert ChatMessage to enum #87

merged 8 commits into from
Aug 15, 2024

Conversation

tjardoo
Copy link
Owner

@tjardoo tjardoo commented Aug 14, 2024

No description provided.

@tjardoo tjardoo changed the title Chat message enum Convert ChaMessage to enum Aug 14, 2024
@tjardoo tjardoo changed the title Convert ChaMessage to enum Convert ChatMessage to enum Aug 14, 2024
Copy link
Contributor

@philpax philpax left a comment

Choose a reason for hiding this comment

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

Looks great! Apologies for misnaming the Clippy step, should've taken a closer look at the copy-paste 😂

You may also want to update DeltaChatMessage; I think you'd be able to delete the Role enum then?

@tjardoo
Copy link
Owner Author

tjardoo commented Aug 14, 2024

I converted the DeltaChatMessage to an enum. The first delta response contains the role. All subsequent responses don't contain the role anymore - therefore I had to add the enum variant 'Untagged'. Any suggestions/alternatives to make it better? Thanks

@tjardoo tjardoo requested a review from philpax August 14, 2024 15:04
@philpax
Copy link
Contributor

philpax commented Aug 14, 2024

Hmm, yeah, that's not ideal. If you don't mind doing some bookkeeping on the user's behalf, you could store the last role received in the stream, and then when you receive the next message, you could deserialize the JSON to a serde_json::Value, inject the role into the value, and then deserialize that value as a DeltaChatMessage. It's a little messy, but it means you won't have to expose an Untagged to the user, and all downstream code can behave appropriately.

Alternatively, you could leave it as-is, but provide a stream adapter that does the above. This would allow users to opt-in to the simplified API as desired, but it does mean you'd still have an Untagged case in there.

A tough choice; I'd lean towards the first, because it does simplify things, even if it obscures what the API is actually returning.

@tjardoo
Copy link
Owner Author

tjardoo commented Aug 15, 2024

let stream = client.chat().create_stream(parameters).await.unwrap();

let mut tracked_stream = RoleTrackingStream::new(stream);

while let Some(response) = tracked_stream.next().await {

@philpax I've added a role tracking stream that that adds the 'role' so the Untagged variant is not needed on the client-side. I'm still doubting whether to make it the default - such that the create_stream() returns the role tracking stream by default. What do you think of this setup?

@philpax
Copy link
Contributor

philpax commented Aug 15, 2024

Seems pretty reasonable to me! I'd go with this for now and see how people feel about it. We're not currently using streaming, so I don't have a strong preference, but this looks like a good solution.

The only thing I'd suggest is adding documentation to clarify what it's for on both RoleTrackingStream and DeltaChatMessage::Untagged, so that users can figure out that it's available from looking at the docs / looking at how to handle Untagged in their editor.

@tjardoo tjardoo marked this pull request as ready for review August 15, 2024 19:20
@tjardoo tjardoo merged commit d1be8df into master Aug 15, 2024
6 checks passed
@tjardoo tjardoo deleted the chat-message-enum branch October 2, 2024 13:31
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.

2 participants