jdiegodcp / ramlfications

Python parser for RAML
https://ramlfications.readthedocs.org
Apache License 2.0
234 stars 50 forks source link

fixed parsing responses bodies, see spotify/ramlfications#12 #13

Closed jenner closed 9 years ago

jenner commented 9 years ago
econchick commented 9 years ago

Hey @jenner! Thank you so much! I'll review this PR this week; nice catch!

econchick commented 9 years ago

Still reviewing this - but just a couple of things:

1 I would add/update a test in test_parser.py that parser.py returns what you expected. For example, when parsing the response schema/example in lines 515-521, you actually get (for the example) an array (I think? not too familiar with xml/xsd) of thingies, named Foo and Bar. Essentially similar to the JSON example here.

  1. The !!null use case isn't tested - mind adding a test for that too?
jenner commented 9 years ago

yep, can do

On Tue, May 26, 2015 at 8:06 PM, Lynn Root notifications@github.com wrote:

Still reviewing this - but just a couple of things:

1 I would add/update a test in test_parser.py that parser.py returns what you expected. For example, when parsing the response schema/example in lines 515-521, you actually get (for the example) an array (I think? not too familiar with xml/xsd) of thingies, named Foo and Bar. Essentially similar to the JSON example here https://github.com/spotify/ramlfications/blob/521b8a00ee2212df34d2999c6ecd2012adf39b3f/tests/test_parser.py#L106 .

  1. The !!null use case isn't tested - mind adding a test for that too?

— Reply to this email directly or view it on GitHub https://github.com/spotify/ramlfications/pull/13#issuecomment-105620950.

econchick commented 9 years ago

Merged locally - thank you!

econchick commented 9 years ago

Also FYI will probably upload new release today or tomorrow - cheers!

econchick commented 9 years ago

FYI included in release 0.1.5 and available on PyPI