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

docs(openapi): add postman warning #851

Draft
wants to merge 15 commits into
base: next
Choose a base branch
from

Conversation

t-tullis
Copy link
Contributor

@t-tullis t-tullis commented Aug 2, 2023

🚥 Resolves CX-364

🧰 Changes

Describe in detail what this PR is for.

🧬 QA & Testing

Provide as much information as you can on how to test what you've done.

@t-tullis t-tullis requested a review from kanadgupta September 29, 2023 22:46
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

great work so far, for real! the code is in the exact spot it should be and you've absolutely nailed the logic — i have a few small suggestions below, and i think it might be good to add a test to ensure that it shows up when we want it to. happy to pair on this!

Copy link
Member

Choose a reason for hiding this comment

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

Mind reverting this change?

);
break;
default:
debug(`Type ${chalk.yellow('rdme help')} to see all commands`);
Copy link
Member

Choose a reason for hiding this comment

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

don't think you need this debug statement, mind removing it?

switch (command) {
case 'openapi':
warn(
'You are attempting to upload a Postman collection. This feature is currently experimental. For more information, visit our docs here: https://docs.readme.com/main/docs/openapi#the-api-reference',
Copy link
Member

Choose a reason for hiding this comment

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

small refactoring suggestion: mind creating a separate variable above (called postmanWarning or something like that) and re-using that in all of these warn statements?

Comment on lines +215 to +217
if (definitionVersion.specification === 'postman') {
switch (command) {
case 'openapi':
Copy link
Member

Choose a reason for hiding this comment

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

great job here with this if/switch logic — super readable and it's in the perfect spot 👏🏽

@kanadgupta kanadgupta added documentation Improvements or additions to documentation enhancement New feature or request command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands labels Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants