-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create tip tap documents #97
Conversation
I will be extending this pattern and using code does not quite fit in my mind
Considering a record not found as an Error to improve upstream ergonomics
664888a
to
fd1887f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really great Caleb, thank you. I'm very excited about having this functionality in place.
Left several suggestions, questions and reflections. Let me know if you have any questions.
|
||
// Tiptap's API will return a 200 for successful creation of a new document | ||
// and will return a 409 if the document already exists. We consider both "successful". | ||
if response.status().is_success() || response.status().as_u16() == 409 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to suggest turning the 409 into some TipTap return code type that we can reuse throughout that codifies TipTap's return codes that we use? That'll help us better document what the return code means with a name. Maybe an enum that stores an u16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a great idea. Let me think a bit on where that might live? At some point I think we'll likely have a dedicated Tiptap module that will encapsulate everything Tiptap REST API related. An enum describing the various semantic meanings of Tiptap's HTTP statuses would definitely belong there. I decided against preemptively defining a module like that in this PR as I figured the design would be premature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense. I guess that's why I am suggesting for now, just a really simple moving of the TipTap-related items into a file like tiptap_client.rs
or something is helpful but not a big lift. It's helpful to separate your TipTap functions out from coaching_session.rs but not have to restructure it as a fancy struct just yet. I'm open to your thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that totally works. I'll make that update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a tiptap
module in 82fe4be
Decided to hold off on defining the enum for responses as it will require me to map all possible reqwest::STATUS_CODE
variants to whatever enum we define. We can come back around when we have a better idea of what the semantic meaning of each is for tiptap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of enhancements for the CLI —help console output.
Co-authored-by: Jim Hodapp <[email protected]>
Co-authored-by: Jim Hodapp <[email protected]>
Clap was complaining and just having a -t seems not super expressive
Description
This PR adds the creation of Tiptap documents when a new
coaching_sessions
record is created.It also refactors and modifies a few other patterns in the application.
Note: This one is somewhat beefy. It may be helpful to review commit by commit.
GitHub Issue: Part of epic issue #96
Changes
Testing Strategy
I tested this using Utoipa
TIP_TAP_URL=https://{your tiptap app ID}.collab.tiptap.cloud TIP_TAP_AUTH_KEY={your tiptap API key} cargo run
POST /coaching_sessions
request from Utoipa with a body similar to:Concerns
Name Changes
What's Next