Recursive Include Support #8
Conversation
|
Before I put any more work into this (tests or whatever), I wanted to open this to see if this was a change that maintainers would be down with. |
|
Hey @benhamill - thanks so much for starting this discussion! And I feel like I should give you a door prize or something since you're the first to raise an issue/make a PR on this young project \o/ Re-reading the spec, it isn't explicit about cyclic/recursive You're definitely right that this parser breaks if there's an include within an include. And should be handled either by honoring them, or by raising an error. Without clarification/clear stance from the spec, I'm inclined to say that'd it'd be easier to raise an error since it could get pretty hairy. However, if you have the energy/time to add this functionality, I'd welcome it. With regards to file types - a RAML parser is indeed supposed to be able to handle I'm queuing up a patch to at least raise an error if there are |
|
Rockin'. At work, we're definitely going to want to use nested I was getting an explosion when the parser was trying to load JSON that I'd |
benhamill
force-pushed
the
recursive_include_support
branch
2 times, most recently
from
5db373c
to
700f7ea
Compare
|
So this is OK, except for the Python 3 tests. Python's a new language to me, so I'm not sure what the deal there, is... A lot of tests broke that don't seem directly related to my change, but I guess they still go through the loader. Any advice? |
|
this is so weird - I am getting similar errors when pushing to my own fork and travis builds, but all run fine locally... I'm investing this further. I'll make some comments on your PR for now, too. Thanks! |
| @@ -28,7 +28,10 @@ def _yaml_include(self, loader, node): | |||
| file_name = os.path.join(os.path.dirname(loader.name), node.value) | |||
|
|
|||
| with open(file_name) as inputfile: | |||
| return yaml.load(inputfile) | |||
| if file_name.endswith(('yaml', 'raml', 'json')): | |||
There was a problem hiding this comment.
yml is also a viable yaml file extension
|
Weird. Cool. Thanks! I appreciate your time. |
benhamill
force-pushed
the
recursive_include_support
branch
2 times, most recently
from
3b85703
to
f7b0245
Compare
| @@ -28,7 +28,7 @@ def _yaml_include(self, loader, node): | |||
| file_name = os.path.join(os.path.dirname(loader.name), node.value) | |||
|
|
|||
| with open(file_name) as inputfile: | |||
| return yaml.load(inputfile) | |||
| return yaml.load(inputfile, self._ordered_loader) | |||
There was a problem hiding this comment.
This got a lot simpler, since the OrderedLoader is based on yaml.SafeLoader already. But we have to send it to the OrderedLoader or else it breaks the recursion.
Maybe that's a sign that we should change how OrderedLoader is created?
|
Hey @benhamill - I was able to figure out the failure issues with py33/34. Apparently the default posix setting is This patch also includes error handling for recursive includes - something as an intermediary until we can get your feature to work (will upload a release to PyPI later today). Now time to dig into how |
benhamill
force-pushed
the
recursive_include_support
branch
from
f7b0245
to
56230dc
Compare
|
❤️ I really appreciate how responsive you've been with this. Looks like my tests are broken. I'm surprised the Then it'll only be about refactor. 😀 |
Right now, the support for `!include` only works at the first level and only supports including yaml (and json) files. This change should make it so that if you have a raml file with an include that pulls in a file that also has an include, it'll keep walking that tree. Also, it looks at the file extension and if it isn't `yaml`, `raml` or `json`, it assumes you just want to pull it in as a string, rather than make any attempt to parse it.
benhamill
force-pushed
the
recursive_include_support
branch
from
56230dc
to
5822b6f
Compare
To guard against regression, and prove functionality, added a test and associated example data to make sure nested `!include` directives work and a test to make sure loading non-yaml and non-json files works.
benhamill
force-pushed
the
recursive_include_support
branch
from
5822b6f
to
c034652
Compare
|
Yaaaaaaay! The build is green. Lemme know what you think about refactors. |
|
Bump bump. @econchick: I don't want to annoy, so I won't bump again. Just wanting to make sure you were busy rather than forgetful. I'll just keep my eye on my inbox for any reply here. |
|
Hey @benhamill ! definitely busy just the past few days; but I assure you this is a "task" on my "committed this week" list. Thanks for the polite nudge! |
|
Ugh so sorry - did not have the capacity last week but I assure you this PR is not forgotten! |
|
No worries. We're using my branch to more forward, so I'm not blocked on your free time or anything. I'm collecting a few other ideas I may propose to you in a few Issues to see how you like them, but I'm still making sure I understand my problems before I propose solution to them. 😀 |
|
Merged! Thanks so much! This works great. I'll release another update to PyPI & GitHub this week that includes these changes. Much appreciated for your help, and looking forward to hearing your ideas. |
Right now, the support for
!includeonly works at the first level and only supports including yaml (and json) files.This change should make it so that if you have a raml file with an include that pulls in a file that also has an include, it'll keep walking that tree.
Also, it looks at the file extension and if it isn't
yaml,ramlorjson, it assumes you just want to pull it in as a string, rather than make any attempt to parse it.