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.

Sign up for GitHub

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

Jump to bottom

fix(oas): improve consumes/produces configuration handling #211

Merged
merged 4 commits into from
Sep 21, 2023
Merged

fix(oas): improve consumes/produces configuration handling #211

merged 4 commits into from
Sep 21, 2023

Conversation

ostridm
Copy link
Contributor

@ostridm ostridm commented Sep 18, 2023

closes #210

@ostridm ostridm added the Type: bug Something isn't working. label Sep 18, 2023
@ostridm ostridm self-assigned this Sep 18, 2023
@codeclimate
Copy link

codeclimate bot commented Sep 18, 2023 •
edited
Loading

Code Climate has analyzed commit 29d656e and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 6

The test coverage on the diff in this pull request is 96.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 91.8% (0.0% change).

View more on Code Climate.

@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch from 1041454 to 6bd7082 Compare September 18, 2023 16:00
@ostridm ostridm enabled auto-merge (squash) September 18, 2023 16:07
@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch from b17bb25 to 2dedf06 Compare September 19, 2023 05:45
@ostridm ostridm marked this pull request as draft September 20, 2023 12:27
auto-merge was automatically disabled September 20, 2023 12:27

Pull request was converted to draft

@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch 7 times, most recently from 766f309 to efa283e Compare September 21, 2023 07:18
@ostridm ostridm marked this pull request as ready for review September 21, 2023 07:20
@ostridm ostridm requested a review from pmstss September 21, 2023 07:21
@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch from efa283e to f9592b5 Compare September 21, 2023 07:36
@ostridm ostridm enabled auto-merge (squash) September 21, 2023 07:36
@ostridm ostridm changed the title fix(oas): use application/json when consumes media-type configuration missing fix(oas): use default media-type when consumes/produces configuration missing Sep 21, 2023
@ostridm ostridm changed the title fix(oas): use default media-type when consumes/produces configuration missing fix(oas): improve consumes/produces configuration handling Sep 21, 2023
pmstss
pmstss requested changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@pmstss pmstss left a comment

Choose a reason for hiding this comment

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

thanks, looks good, please consider following improvement in addition to commented in code:

  • please add explicit root level consumes fixture
  • please add fixtures for produces cases (root-level, operation-level, overriding)

packages/oas/src/converter/parts/Oas2MediaTypesResolver.ts Outdated Show resolved Hide resolved
packages/oas/src/converter/parts/headers/HeadersConverter.ts Outdated Show resolved Hide resolved
@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch 3 times, most recently from 936fb2e to 02a75e3 Compare September 21, 2023 12:24
@ostridm ostridm force-pushed the fix-#210/use-application-json-when-consumes-configuration-missing branch from 02a75e3 to 29d656e Compare September 21, 2023 12:28
@ostridm ostridm requested a review from pmstss September 21, 2023 12:31
@ostridm ostridm merged commit d7a3cbe into master Sep 21, 2023
4 checks passed
@ostridm ostridm deleted the fix-#210/use-application-json-when-consumes-configuration-missing branch September 21, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmstss pmstss pmstss approved these changes

@derevnjuk derevnjuk derevnjuk approved these changes

Assignees

@ostridm ostridm

Labels
Type: bug Something isn't working.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OAS: Missing media-type configuration leads to swagger converter error
3 participants
@ostridm @pmstss @derevnjuk