-
Notifications
You must be signed in to change notification settings - Fork 108
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
Api #3367
Api #3367
Conversation
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.
Looks generally good. I have a suggestion or two, and a question or two.
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.
👍
Sigh; and, once again, I had to nudge Mr Jenkins to wake him up because Mr Smee slept through his alarm again ... |
PBENCH-1119 With the REST-ification of the API, only a few of the APIs can still be called using the simplistic `{name: uri}` list, because URI parameters are needed and are no longer at the end of the URI: for example, `/api/v1/datasets/{dataset}/metadata`. The UI now has the `uriTemplate` method to help expand the `"uri"` template objects. Complete the transition by updating the remaining UI references and dropping the `api` object entirely.
(How'd that happen?)
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.
👍
@@ -1,13 +1,15 @@ | |||
import * as TYPES from "./types"; | |||
|
|||
import API from "../utils/axiosInstance"; | |||
import { uriTemplate } from "utils/helper"; |
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.
Nit: overviewActions.js
and tableOfContentActions.js
import from ../utils/helper
(although findNoOfDays
is imported from utils/dateFunctions
in overviewActions.js
). Assuming that they are equivalent, maybe we should go with a single idiom in all cases.
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.
It's a good point, and I'm not quite sure how this ended up that way because I'm usually not inclined to depend on the default root. (And it confuses vscode
, too.)
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.
LGTM (with one nit that we might want to resolve eventually).
PBENCH-1119
With the REST-ification of the API, only a few of the APIs can still be called using the simplistic
"api": {name: string, ...}
list, because URI parameters are generally needed and are no longer at the end of the URI: for example,/api/v1/datasets/{dataset}/metadata
.The UI now has the
uriTemplate
method to help expand the"uri"
template objects. Complete the transition by updating the remaining UI references and dropping theapi
object entirely.This was immediately motivated by Webb's comment on my API documentation PR that it was odd I was documenting that the
api
object inendpoints
is "mostly obsolete"; better to simply remove it and move on.