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 35; affects [npm] #9866

Merged
merged 2 commits into from
Jan 6, 2024

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jan 1, 2024

Refs #9285

@chris48s chris48s added service-badge New or updated service badge documentation Developer and end-user documentation labels Jan 1, 2024
Copy link
Contributor

github-actions bot commented Jan 1, 2024

Warnings
⚠️ This PR modified service code for npm 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 0e8a10a

Copy link
Contributor

github-actions bot commented Jan 3, 2024

🚀 Updated review app: https://pr-9866-badges-shields.fly.dev

Comment on lines +19 to +32
'/npm/dependency-version/{packageName}/{dependency}': {
get: {
summary: 'NPM (prod) Dependency Version',
parameters: [
pathParam({
name: 'packageName',
example: 'react-boxplot',
description: packageNameDescription,
}),
pathParam({
name: 'dependency',
example: 'simple-statistics',
description: packageNameDescription,
}),
Copy link
Member

Choose a reason for hiding this comment

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

I like the fact that we've simplified this into two fields with an explanation of what this scoping business is. I'm personally somewhat familiar with the "scope" terminology because I've occasionally come across it in the Shields.io code, but I feel that someone which isn't a JS power-user may not be familiar with the wording.

However, I must admit that I was briefly confused both when reading the code and trying the badge out in the review app. I think this boils down to the fact that package is a package and dependency is also a package, and we're basically overloading the word package in the path parameter name, the explanation of the parameters, and the examples within the explanations.

I think it may help to replace any mentions of package by dependency in the dependency parameter description (even if a dependency is technically also a package), and instead of giving package and @author/package as examples in the description, use something more specific-sounding like my-package and @author/my-package

@chris48s chris48s added this pull request to the merge queue Jan 6, 2024
Merged via the queue into badges:master with commit 9493d00 Jan 6, 2024
21 checks passed
@chris48s chris48s deleted the 9285-part35 branch January 6, 2024 16:46
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