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

feat: response-contains-property built-in rule #535

Closed
wants to merge 1 commit into from

Conversation

andriyl
Copy link
Contributor

@andriyl andriyl commented Jan 27, 2022

What/Why/How?

Closes #389

Check yourself

  • Code is linted
  • Tested with redoc/reference-docs/workflows
  • All new/updated code is covered with tests

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@andriyl andriyl self-assigned this Jan 27, 2022
@andriyl andriyl marked this pull request as ready for review January 27, 2022 16:43
return {
Response: {
skip: (_response, key) => {
return !['200', '201', '202'].includes(key.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these numbers mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tatomyr
HTTP status codes, we omit all statuses except those listed above, because those requests have no response content.

Copy link
Member

Choose a reason for hiding this comment

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

Well, they may have response content but may be errors, and that's why they aren't applicable either.

204 - no content
4XX - bad requests
5XX - server errors

It's common that things like 4XX have different properties.

Copy link
Member

Choose a reason for hiding this comment

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

Same as #537 (comment)

But here also different mime types are in play, so I'm not sure what to suggest yet.

@adamaltman do you have some ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think the 2XX, 4XX configuration options is a good idea.

@tatomyr tatomyr assigned tatomyr and unassigned andriyl Mar 21, 2022
@tatomyr
Copy link
Contributor

tatomyr commented Mar 31, 2022

Closing this since the changes are incorporated into #537

@tatomyr tatomyr closed this Mar 31, 2022
@tatomyr tatomyr deleted the feat/response-contains-property-rule branch May 31, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

response-contains-property built-in rule
4 participants