-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(api): improvements to the installation screen #783
Conversation
})); | ||
} | ||
.action(async (uri: string, options: Options) => { | ||
const language = await getLanguage(options); |
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.
Just some very small refactors to move this language
and identifier
selection out so this command action is a little easier to read.
return false; | ||
} | ||
|
||
const snippet = client.convert( |
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.
I don't love this but I also really don't want to have to make @readme/oas-to-snippet
a dependency because that depends on httpsnippet-client-api
and if we need to make fixes to this snippet generation work it's be a multistep process:
- publishing
httpsnippet-client-api
- pulling that release into
@readme/oas-to-snippet
- publishing
@readme/oas-to-snippet
. - puling that release into
api
- publishing
api
.
@@ -47,10 +47,10 @@ function buildAuthSnippet(authKey: string | string[]) { | |||
auth.push(`'${token.replace(/'/g, "\\'")}'`); | |||
}); | |||
|
|||
return `sdk.auth(${auth.join(', ')});`; | |||
return `${sdkVariable}.auth(${auth.join(', ')});`; |
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.
found a bug in these .auth()
snippets with custom variables while building this out.
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.
changes look good, really liking some of the refactors here! one small wrinkle: if the user's identifier has a hyphen, we should probably convert it to camelcase (or underscores, whatever you think makes the most sense!)
- import transit-new from '@api/transit-new';
+ import transitNew from '@api/transit-new';
- transit-new.getNextripAgencies()
+ transitNew.getNextripAgencies()
.then(({ data }) => console.log(data))
.catch(err => console.error(err));
if (isReservedOrBuiltinsLC(sdkVariable)) { | ||
// If this identifier is a reserved JS word then we should prefix it with an underscore so | ||
// this snippet can be valid code. | ||
sdkVariable = `_${sdkVariable}`; |
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.
great call here
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.
yeah we do something similar for method and type names in codegen
🧰 Changes
This work is some QOL improvements to the post-installation screen where we'll now, using a very simple algorithm to determine the best endpoint for a demo1, show a code example to folks so they can immediately get started with their newly generated SDK. Here's what it looks like:
And in the case of us not being able to find a good endpoint to demo2 we just won't show a code example:
Footnotes
With the exception of us being able to handle auth examples, this algorithm is the same that powers the "recommended" API endpoint section of ReadMe Realtime configuration. ↩
In this case this API definition was only comprised endpoints with path parameters. ↩