Skip to content

Conversation

@ymohdriz
Copy link
Contributor

@ymohdriz ymohdriz commented Jul 2, 2018

Hi team,

This PR is to solve the illegal argument exception in swagger converter as mentioned in the issue #740

Please review the PR and merge the same.

Thanks,
Mohammed

ymohdriz added 3 commits July 2, 2018 18:27
OpenAPI v2 converter: java.lang.IllegalArgumentException with allOf and discriminator
OpenAPI v2 converter: java.lang.IllegalArgumentException with allOf and discriminator
ymohdriz added 2 commits July 2, 2018 22:13
ResolveFully doesn't resolve the request bodies
@jmini
Copy link
Contributor

jmini commented Jul 3, 2018

Thank you for looking into this topic.

I am not part of the team, but I guess a unit test to cover your changes would be great.

Have a look at V2ConverterTest. You will need to add a new file like issue-740.yaml in swagger-parser-v2-converter/src/test/resources and a new test case in V2ConverterTest.

The example (issue-740.yaml) should be as small as possible.
Your test should be red before your change and should be green after that.

Let me know if you need some help.

@jmini
Copy link
Contributor

jmini commented Jul 11, 2018

This change looks good to me.

@ymohdriz Thank you for this contribution.

I recommend you to add a unit test to this PR.

As an example, I have created one jmini@da64e41 in my fork on top of your commit ymohdriz@e4da496.

Can you please integrate this commit (or a similar one) in this Pull Request?


Note: When merging this PR I recommend "Squash and merge" because the intermediate commit in this branch are not really useful.

@ralphdoe
Copy link
Contributor

ralphdoe commented Jul 13, 2018

Like @jmini pointed. I think it might be a good idea to add the test. Thanks for your contribution @ymohdriz

@ymohdriz
Copy link
Contributor Author

Hi @ralphdoe,

Sure I will add the junit test for this issue. In future, I will commit the junits along with the fix.

Thanks,
Mohammed

jmini added a commit to OpenAPITools/swagger-parser that referenced this pull request Jul 13, 2018
@jmini
Copy link
Contributor

jmini commented Jul 19, 2018

This PR has still no junit tests...

@ymohdriz feel free to just add my commit to your PR or to add something similar.
Thank you in advance.

@jmini
Copy link
Contributor

jmini commented Jul 23, 2018

@ralphdoe, @gracekarina: I have opened an alternative PR to include the unit test discussed here and to solve the merge conflicts. => #777

@ralphdoe ralphdoe merged commit e4da496 into swagger-api:2.0 Jul 25, 2018
@ymohdriz ymohdriz deleted the branch_v2.0.1 branch July 25, 2018 05:25
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.

3 participants