Convert examples arrays to openApi objects (part 1)
#9320
Conversation
- make description optional - allow null example + allowEmptyValue (for boolean query params)
we can use these for all the 'test results' badges
chris48s
added
core
|
| staticPreview: this.render({ format: 'rating', rating: 4 }), | ||
| keywords, | ||
| static openApi = { | ||
| '/amo/rating/{addonId}': { |
There was a problem hiding this comment.
There is some duplication here between the route definition and the object keys here. My assumption was this should be fixable (refs #8966 (comment) ). Unfortunately this proved hard to fix because we can't use an expression here. These object keys can only be a literal. So no function calls, template strings, etc here. I think if we want to use bits of the route definition, we need to make openApi a function rather than a static class property. My gut instinct is that accepting the duplication is actually the lesser evil here, but open to thoughts on it.
There was a problem hiding this comment.
I'm currently ambivalent on it. In cases like this where we had an enum-like route param in the pattern it doesn't seem like much of a change from what we were doing before.
However, in cases where we have examples relying on the previously defined route it does feel like we're introducing duplication. I think I can live with that though, so long as we've still got something automated that can be used to validate the validity of these routes, and be easy to check both locally and CI.
I.e. If we ever have a small PR that updates a route path, I don't want to have to rely on human reviewers to remember to check the spec (which may or may not be part of the diff)
There was a problem hiding this comment.
Hmm. So maybe this wasn't the right place to open this discussion point.
If we take format(stars|rating) out of the equation for a moment:
What I'm really getting at here is that writing something like
static openApi = {
'/' + this.route.base + '/{param}': //some object
}or
static openApi = {
`/${this.route.base}/{param}`: //some object
}is not legal javascript. We'd have to make the openApi object something that is returned from a function call to do something like that. That was the original point.
Then if we re-introduce format(stars|rating) to the conversation.
Basically the reason why I've done this like this is because that's how it was before: It was a one-to-one replacement. Also partly because I've not really fully tackled enums yet.
However, instead of writing:
static openApi = {
'/amo/rating/{addonId}': {
get: {
summary: 'Mozilla Add-on Rating',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
'/amo/stars/{addonId}': {
get: {
summary: 'Mozilla Add-on Stars',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
}We could write this instead:
static openApi = {
'/amo/{format}/{addonId}': {
get: {
summary: 'Mozilla Add-on Rating',
parameters: [
pathParam({
name: 'format',
example: 'stars',
schema: { type: 'string', enum: ['stars', 'rating'] },
}),
pathParam({ name: 'addonId', example: 'dustman' }),
],
},
},
}..and then I think (untested) we should be able to add another helper to derive the allowed enum values from the route. Tbc. As I say, I've not really tackled this case yet.
We could say we always represent a :param(this|that) as an enum and only use multiple examples when we have an optional param (e.g: :branch*). I guess which of those options we prefer is partly a stylistic decision though, or what is most useful to show the user. As opposed to always one-to-one mapping with the route.
There was a problem hiding this comment.
Do you feel like the latter option might make it easier for us to more have a single example in the badge listing (e.g. a single listing for a downloads badge with the interval param vs. a listing per interval) if/when we want?
Or, do you anticipate we'd like to maintain individual listings per variant and then keep that component of the route static in each of the corresponding builders?
There was a problem hiding this comment.
I think in most cases, probably optimising for fewer different pages/menu items in the menu on the left with more options to customise is going to be the better option.
|
🚀 Updated review app: https://pr-9320-badges-shields.fly.dev |
calebcartwright
added
the
service-badge
| staticPreview: this.render({ format: 'rating', rating: 4 }), | ||
| keywords, | ||
| static openApi = { | ||
| '/amo/rating/{addonId}': { |
There was a problem hiding this comment.
I'm currently ambivalent on it. In cases like this where we had an enum-like route param in the pattern it doesn't seem like much of a change from what we were doing before.
However, in cases where we have examples relying on the previously defined route it does feel like we're introducing duplication. I think I can live with that though, so long as we've still got something automated that can be used to validate the validity of these routes, and be easy to check both locally and CI.
I.e. If we ever have a small PR that updates a route path, I don't want to have to rely on human reviewers to remember to check the spec (which may or may not be part of the diff)
| parameters: [ | ||
| pathParam({ name: 'user', example: 'gruntjs' }), | ||
| pathParam({ name: 'repo', example: 'grunt' }), | ||
| pathParam({ name: 'branch', example: 'master' }), | ||
| ], |
There was a problem hiding this comment.
This has me wondering if it'd be worth either adding another wrapper function around pathParam that takes an array, e.g. pathParams that'll iterate over the entry args and contain the call sites to pathParam.
That in turn would allow this to be transformed into something like:
| parameters: [ | |
| pathParam({ name: 'user', example: 'gruntjs' }), | |
| pathParam({ name: 'repo', example: 'grunt' }), | |
| pathParam({ name: 'branch', example: 'master' }), | |
| ], | |
| parameters: pathParams([ | |
| { name: 'user', example: 'gruntjs' }, | |
| { name: 'repo', example: 'grunt' }, | |
| { name: 'branch', example: 'master' }, | |
| ]), |
My thinking is that might help minimize some of the rightward drift (fewer characters/columns after all), and the potential for prettier to want to do some multi-line wrapping to account for the width.
Thoughts?
There was a problem hiding this comment.
Yes. Love it!
I'll add these for both pathParams() and queryParams().
Having got into this...
One implementation of this would be that the plural variants take an array as you've suggested.
So the implementation would look like:
function pathParams(params) {
return params.map(param => pathParam(param))
}..and then calling that would look like:
pathParams([{ name: 'onlyParam', example: 'onlyValue' }])
pathParams([
{ name: 'param1', example: 'value1' },
{ name: 'param2', example: 'value2' },
])The other option would be to have the plural variants take a spread as input instead of an array.
So then the implementation would look like:
function pathParams(...params) {
return params.map(param => pathParam(param))
}..and calling that would look like:
pathParams({ name: 'onlyParam', example: 'onlyValue' })
pathParams(
{ name: 'param1', example: 'value1' },
{ name: 'param2', example: 'value2' },
)Having thought about it a bit, I prefer the second variant myself. What do you reckon?
Note to self: When you finish this off, these should also have DocStrings!
There was a problem hiding this comment.
Sure, second approach is my preference too 👍
|
Thanks for providing feedback on this. There's some useful points here. I'll try to get into them in the next week or so. |
|
I'm aware I also have merge conflicts from the prettier update to deal with on this PR before it is merge-able |
|
Hmm. For some reason builds do not appear to be running on this PR. Maybe merge conflicts? I have more commits to push to finish this so I will sort it then. |
|
This one has been sitting around for a while, and I'd like to get some momentum back into this project. So I have made some decisions and pushed some commits:
I have also decided to scope creep this a little: For the moment I am going to leave this open, but if I've got more time to work on this again in the next few days, I might just go rogue and merge it so I can make some more progress. |
|
Sorry for the delay on a follow up review and response to discussion. It's on my list for this weekend but I've got some #day-job stuff that's likely going to eat up a lot of my free time this weekend unfortunately |
I think it's good to go as-is tbh. The only semi-open thing from my POV would be #9320 (comment) but as you noted it's more of a stylistic thing, and one which I think we could discuss elsewhere/in parallel to decide if/when it's something we want to do (I don't think this and all the other conversions need to be blocked on that) |
I've been umming and aahing about how best to start approaching #9285
My first thought was that I would try and write a script to auto-convert everything, and then tweak from there, but I think that is going to end up with making several enormous PRs that touch ~400 files. I have written some very rough scripts to help with this process and give me a starting point. That said, having got this deep into it, I've decided that I'm just going to tackle this in small to medium-sized batches of services (auto-converting first where possible, then manually reviewing and tweaking from there) rather than multiple passes across the whole codebase. I will also look at adding appropriate tooling and abstractions to handle various cases as I go along and meet them.
This is batch one of... many. This one also establishes some of the functions we are going to need moving forward.