jdiegodcp / ramlfications

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

Only the first method of a resource defined in an external file is detected #85

Closed bpowell65536 closed 8 years ago

bpowell65536 commented 8 years ago

I'm trying to define a resource type in an external RAML file, but when ramlfications parses it, only the first of several access methods (get/post/patch/etc.) are detected. A minimal set of RAML files illustrating the problem are attached (GitHub doesn't support uploading .raml files, so the extensions need to be changed).

externalResource.raml.txt resource_with_multiple_methods.raml.txt

When I parse 'resource_with_multiple_methods.raml', the result has four resources - get, patch and post for the /internal path, but only get for the /external path. I would have expected 'get', 'post' and 'patch' methods for both paths.

>>> from ramlfications import parse
>>> spec = parse("resource_with_multiple_methods.raml")
>>> spec.resources
[ResourceNode(method='get', path='/internal'), ResourceNode(method='post', path='/internal'), ResourceNode(method='patch', path='/internal'), ResourceNode(method='get', path='/external')]

I reproduced this behaviour with ramlfications 0.1.9.

Any help would be greatly appreciated.

bpowell65536 commented 8 years ago

I have developed a test case and a fix for this issue based on the RAML files included in the issue description in the 0.2.0-dev branch (the problem exists in 0.2.0-dev as well as 0.1.9), but unfortunately the fix has broken the tests/test_parser.py::test_resource_inherits_type test case, and I don't understand enough of how the parsing works to fix what I've broken. Would someone who knows more about how the parsing works be able to take a look at what I've done? Do I need to post a pull request?

econchick commented 8 years ago

@bpowell65536 hey sorry I didn't originally respond. You don't have to make a PR, but it'd be great if you could push it to a branch on your fork, then I can take a look at it.

bpowell65536 commented 8 years ago

@econchick Thanks for your response :). I have pushed my changes to the v0.2.0-dev branch on my fork of ramlfications, which is here: https://github.com/bpowell65536/ramlfications/tree/v0.2.0-dev/ramlfications . The changed implementation file is ramlfications/parser/main.py, the associated tests are in tests/test_parser.py and the test data files are tests/data/examples/external_resource_with_multiple_methods.raml, tests/data/examples/extended_external_resource_with_multiple_methods.raml, tests/data/examples/includes/external-resource.raml and tests/data/examples/includes/extended-external-resource.raml.

Apologies for all the logging statements - I will remove those when the fix is done.

bpowell65536 commented 8 years ago

I think I have fixed the problem. I have run tox on my version of the code and all the tests are passing on Python 2.6.9, 2.7.6, 3.3.6, 3.4.3 and pypy 2.2.1, except tests/test_tree.py:test_print_tree_light_v, tests/test_tree.py:test_print_tree_light_vv, and tests/test_tree.py:test_print_tree_light_vvv, which were marked as skipped in the original checkout of the code. I can make a pull request now if you like.

econchick commented 8 years ago

Sure, feel free to make a PR.

bpowell65536 commented 8 years ago

OK, done. PR is here: https://github.com/spotify/ramlfications/pull/88

bpowell65536 commented 8 years ago

@econchick I have made a new PR with all the previous commits (including changes in response to your comments on PR #88) squashed into one: #91. There is an extra commit because my change broke some of the CI tests - I can try to squash this into the same commit as well if you like.

postatum commented 8 years ago

Hi @econchick :) I'm having the same issue. Originally described it here https://github.com/spotify/ramlfications/issues/23#issuecomment-153009690

Thanks a lot for your PR @bpowell65536!

bpowell65536 commented 8 years ago

@postatum No worries! Hope it helps!

bpowell65536 commented 8 years ago

@econchick This issue didn't really go through the normal process, but since it is fixed now, did you want me to close it?

econchick commented 8 years ago

sure, thanks!