search

jdiegodcp / ramlfications

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

Recursive Include Support #8

Closed benhamill closed 9 years ago

benhamill commented 9 years ago

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 commented 9 years ago

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.

econchick commented 9 years ago

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 !includes. I've actually opened an issue against it hoping for some clarification.

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 yaml and raml, you're right (again!). I'm inclined to also include json as well since the Python yaml library handles it well.

I'm queuing up a patch to at least raise an error if there are !includes beyond the first level, as well as return a pure string for anything not yaml, yml, raml, or json. But I welcome your proposed idea to add that functionality.

benhamill commented 9 years ago

Rockin'. At work, we're definitely going to want to use nested !include because we have a lot of resources and we're going to want to externalize the schemas for them. So, I'm pretty confident I'll have the time and energy to mess with this.

I was getting an explosion when the parser was trying to load JSON that I'd !includeed, so I assumed the loader didn't handle it... but I see, now, the failing test, so it must be another cause. I'll write some tests to sort it out.

benhamill commented 9 years ago

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?

econchick commented 9 years ago

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!

benhamill commented 9 years ago

Weird. Cool. Thanks! I appreciate your time.

econchick commented 9 years ago

Hey @benhamill - I was able to figure out the failure issues with py33/34. Apparently the default posix setting is LANG=C, and should have been set to en_US.utf-8. It worked previously because I suspect fresh containers were not used, and therefore had old (correct) settings. I made a temporary fix by setting the envs, but I'll have to make a patch to force encoding when reading files.

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 OrderedLoader works to answer your question from yesterday!

benhamill commented 9 years ago

:heart: I really appreciate how responsive you've been with this.

Looks like my tests are broken. I'm surprised the "\n" issue didn't pop up before, now that I see it. SMH. And it looks like you added a tests to ensure the error was raised, so I should remove that, now that I rebased. I'll hit those tomorrow at work and hopefully be green, finally.

Then it'll only be about refactor. :grinning:

benhamill commented 9 years ago

Yaaaaaaay! The build is green. Lemme know what you think about refactors.

benhamill commented 9 years ago

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.

econchick commented 9 years ago

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!

econchick commented 9 years ago

Ugh so sorry - did not have the capacity last week but I assure you this PR is not forgotten!

benhamill commented 9 years ago

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. :grinning:

econchick commented 9 years ago

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.