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

feat: Add a new event type:dismiss. Add view_id and elapsed in event body. #1144

Merged

Conversation

icycodes
Copy link
Member

Complete TAB-382

@icycodes icycodes marked this pull request as draft December 29, 2023 11:18
@wsxiaoys
Copy link
Member

wsxiaoys commented Jan 2, 2024

#1148

@wsxiaoys
Copy link
Member

wsxiaoys commented Jan 8, 2024

is this ready for review?

@icycodes icycodes marked this pull request as ready for review January 8, 2024 03:16
@icycodes icycodes requested a review from wsxiaoys January 8, 2024 03:16
@wsxiaoys wsxiaoys requested a review from boxbeam January 8, 2024 03:45
@@ -3,14 +3,18 @@ use utoipa::ToSchema;

#[derive(Serialize, Deserialize, ToSchema, Clone, Debug)]
pub struct LogEventRequest {
/// Event type, should be `view` or `select`.
/// Event type, should be `view`, `select` or `dismiss`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should use an enum to model this, or Discriminant<Event>? I believe I forgot to modify this comment when adding the Completion variant - using Discriminant<Event> would make it clear through the type system what this is, and not require updating the comment every time a new event is added.

Copy link
Member

@wsxiaoys wsxiaoys Jan 8, 2024

Choose a reason for hiding this comment

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

there's no need to modify this, completion event are not logged through this route. (they're part of logic in side /completion)

@wsxiaoys wsxiaoys merged commit af7cbf0 into TabbyML:main Jan 8, 2024
3 checks passed
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.

3 participants