Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(sdk): client, annotations #498

Merged
merged 15 commits into from
Jan 13, 2025
Merged

feat(sdk): client, annotations #498

merged 15 commits into from
Jan 13, 2025

Conversation

doronkopit5
Copy link
Contributor

@doronkopit5 doronkopit5 commented Jan 2, 2025

  • Remove reportScore
  • Introduce TraceloopClient
  • Add userFeedback functionality

@doronkopit5 doronkopit5 marked this pull request as ready for review January 6, 2025 09:48
Copy link
Contributor

@galkleinman galkleinman left a comment

Choose a reason for hiding this comment

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

Looks really clean!

Couple of points to think of:

  • Not sure the package/class I would expect as a user of us is Annotation I mean, I think we should have Annotation as a base class (which actually handles all the logics), and a wrapper which is doing almost nothing except calling the base class with a "user-feedback" flow argument called UserFeeback.
  • EntityInstanceId - I know what the Instance stands for, but maybe we should call it only in the client EntityId? maybe it should be an object in general? Entity { id: , type?: } - think of a robust api since it'll be hard to break it once users will use it...
  • Add some tests :)

Copy link
Contributor

@galkleinman galkleinman 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.

@doronkopit5 doronkopit5 merged commit 6e4192e into main Jan 13, 2025
4 checks passed
@doronkopit5 doronkopit5 deleted the dk/report_labeling branch January 13, 2025 11:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants