-
Notifications
You must be signed in to change notification settings - Fork 212
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
docs(Swagger API): Implement Swagger for API documentation #12266
Conversation
40f7d3e
to
6c62a8c
Compare
17bc3a3
to
508cda0
Compare
508cda0
to
fdb3bcc
Compare
@xlisachan sweeeet on updating them to MD😎 That will also address a lot of Peter's "invalid HTML!" comments. I do think we're going to have to use something like dedent for multi-lines which might not look pretty but that's fine. I'd take some of the most complicated pieces (like with code blocks or list items) and try it out there first. There might be a better tool for the job, not sure. On the separate files, hmmm, I feel like devs would see I'd definitely be fine with pulling all of the descriptors into individual files like my previous comment happening in a follow up, if we wanted to do it at all, but if we're going to do that I kind of also think it might be less time consuming to do it at the same time you change everything to MD since you'll have to touch almost every string again anyway 😩 Maybe before you start on the refactor to MD, we get one or two opinions on what we deem as ideal here. |
b82b61e
to
ada7503
Compare
@xlisachan Still looking at this, phew it's a long one but so much nicer with markdown!! 🙌 Here's more thoughts so far: Sometimes we use the function version of dedent, like
and other times, just
I guess if I have to choose I’d pick the latter because less characters but I really don’t mind either way, but can we make that a bit more consistent? 😀 I’m not sure I immediately know the difference between “🔒🔓 sessionToken” and just “🔒sessionToken”. Can we add a key/legend at the top for what the emojis mean? I still think some sort of constants object could be nice to share common strings, even API headings, but not a must for r+. I could see someone even typoing 'Devices and sessions' and not realizing it because they didn't manually check Swagger. Questions, please feel free to disagree or push back because I could definitely be missing context or something you found:
Potential follow up issues:
|
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.
The account refactor is looking much better separation of concerns wise heh, still feels like I haven't gone through everything but here's a few more comments on your new commit, I might wait before coming back and reviewing again since I didn't realize you were refactoring.
Thanks for the feedback, @LZoog!
Question, should removing the current API documentation be included in this ticket? Thanks again!! :) |
@xlisachan That all sounds great to me! RE: the misc routes, ahh that makes a lot of sense knowing we have #12549. I've already spent a couple of hours on this today and since you're refactoring I'll probably check back on Friday. It makes sense that anything coming from the existing doc is fine for this PR and we can clean up later.
When you say clean up, do you mean pull out the validation as I mentioned previously in this comment where I @ mentioned Dan, or do you mean add to/review our existing descriptions? I think we could use a documentation review ticket too, for stuff like what's pointed out here. 🙂
Eh, if you've copied and pasted everything from them, I think it's fine to delete them here. If there's more details we might should comb over or if it's going to make the diff significantly larger, it can be done in the doc improvement/review follow up. |
I meant the first (your comment). 🙃 |
619e425
to
3bd5de1
Compare
e899cf4
to
c86b6a3
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.
There were some parameters that had two similar descriptions, do we prefer one over the other/want to combine them for a better description?
c86b6a3
to
436736b
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.
@xlisachan Amazing job 👏👏👏 Thanks for tackling this.
Especially after that refactor, things are looking so nice. Almost all of my comments are naming preferences, so those are non-blocking and feel free to disagree.
Might be worth linking the follow up tickets in a comment for posterity. /emails/reminders/cad
can almost certainly go under "emails" but I know it could use a description and I think we've got that "go through misc routes" ticket still.
069c1ed
to
dc8ca1d
Compare
dc8ca1d
to
8699472
Compare
To view documentation
add_hapiswagger
branchyarn install
in fxa-auth-serveryarn start
fxanpm start
in fxa-auth-serverThis pull request
Issue that this pull request solves
Closes: #12051
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
Other information (Optional)
This pull request adds hapi-swagger v10.3.0, as fxa-auth-server is currently utilizing
@hapi/joi 15.1.1
. Once the entire codebase is using joi v17.x, the following dependencies can be updated:Follow-up tickets filed: