Skip to content
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

migrate examples to openApi part 13; affects [curseforge date fedora hsts modrinth ore] #9499

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

chris48s
Copy link
Member

Refs #9285

I'm not going to repeat the same blurb. In this one I am still tackling easier cases (all path params) but I'm now moving on to the ones with a description/documentation string of some kind.

@chris48s chris48s added service-badge New or updated service badge documentation Developer and end-user documentation labels Aug 21, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2023

Warnings
⚠️ This PR modified service code for curseforge but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for date but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for fedora but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for hsts but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for modrinth but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for ore but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 05e7810

keywords: ['time', 'countdown', 'countup', 'moment'],
documentation,
static openApi = {
'/date/{timestamp}': {
Copy link
Member

Choose a reason for hiding this comment

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

I'm just now realizing (or perhaps have just forgotten) that I'm not sure if/how we're doing any parameter validation on the frontend badge builder in the new ui 🤔

More of a reminder/edification question, definitely non-blocking

Copy link
Member Author

@chris48s chris48s Nov 30, 2023

Choose a reason for hiding this comment

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

I think this is the relevant bit of validation:

// ensure openApi spec matches route
if (this.openApi) {
const preparedRoute = prepareRoute(this.route)
for (const [key, value] of Object.entries(this.openApi)) {
let example = key
for (const param of value.get.parameters) {
example = example.replace(`{${param.name}}`, param.example)
}
if (!example.match(preparedRoute.regex)) {
throw new Error(
`Inconsistent Open Api spec and Route found for service ${this.name}`,
)
}
}
}
}

Added in #9320 in response to #9320 (comment)

Does that cover what you were thinking of here?

@chris48s
Copy link
Member Author

chris48s commented Dec 4, 2023

Going to just go ahead and merge this, but feel free to follow up on the comment about validation

@chris48s chris48s added this pull request to the merge queue Dec 4, 2023
Merged via the queue into badges:master with commit 5d1ddbc Dec 4, 2023
22 checks passed
@chris48s chris48s deleted the 9285-part13 branch December 4, 2023 12:58
Lordfirespeed pushed a commit to Lordfirespeed/shields that referenced this pull request Dec 18, 2023
…hsts modrinth ore] (badges#9499)

* migrate some services from examples to openApi

* improve and de-dupe service titles

* improve ore description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Developer and end-user documentation service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants