fix Parsing of Model "example" is not consistent with parsing of Prop… #449
Conversation
boillodmanuel
force-pushed
the
fix/model-example
branch
from
b89b6f0 to
d8131e0
Compare
|
This is a critical fix for me. What do we have to do to move this along? |
|
Does this need to be rebased? Anything else stopping it from being merged? |
|
@boillodmanuel @DaneWeber yes this needs to be rebased. Sorry for the delay. I Will review a soon as rebased 👍 |
| @Test | ||
| public void testIssue255() { | ||
| SwaggerParser parser = new SwaggerParser(); | ||
|
|
There was a problem hiding this comment.
Hi, why delete this test instead of making it work with the new approach?.
There was a problem hiding this comment.
Could I try to make this test work? Again, anything to move this fix along would be great.
There was a problem hiding this comment.
yes, please and rebase as well. Thanks!
There was a problem hiding this comment.
So, grab the pull request, fix the test, rebase against the latest, and resubmit the pull request? You know I'm not the original author of this pull request ...
There was a problem hiding this comment.
oh ok. So how else can we speed this fix?
There was a problem hiding this comment.
I'll try to patch up the pull request and see if I can make the test work. Hopefully sometime later today or tomorrow (unless the original author fixes this first).
There was a problem hiding this comment.
After some work/investigation and consulting the original issue 255, I believe this fix is correct and the original was not (and this is also indicated/hinted at in the fix of 255). There are unit tests in this fix that verify that a proper ArrayModel is returned for the array example (not a string as in the prior fixes unit test). I also tested with several downstream pipelines (one that takes swagger -> markdown -> asciidoc -> pdf and a similar one that takes swagger -> markdown -> asciidoc -> static html). In each case, the fixed version works (i.e. the final pdf / html with an array of objects is displayed correctly). So, the test was removed because it is somewhat redundant. Also, it seems like you might want the original issue 255 authors/collaborators to weigh in as I do not know what their downstream usage of the swagger-parser is (so this fix could break their usage - I cannot tell).
There was a problem hiding this comment.
So, are we good to go? Will this show up soon in a release?
There was a problem hiding this comment.
Hi we need this is updated with master branch, I do not have permission to update the @boillodmanuel fork, let me check what we can do. 👍
|
So, what causes new releases of the swagger parser? Just trying to gauge when this fix will make into an official release and into standard repos ... |
|
@kidnme sorry, didn't see the message before. We try to release when we accumulate enough fixes, or when there's a critical bug fix (normally security related should one exist). We also try to coordinate releases of our libraries together, so it can get a bit more complicated. Regardless, we've made a release today, feel free to jump on it! |
5 participants
…erty "example" #448