jdiegodcp / ramlfications

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

Collecting All Validation Errors on .validate() Instead of Raising Exception #25

Closed cerivera closed 8 years ago

cerivera commented 8 years ago

Addresses: https://github.com/spotify/ramlfications/issues/21

New usage:

errors = ramlfications.validate(RAML_FILE)
if errors:
    for error in errors:
        print(error)
else:
    print("Success!")

ramlfications.parse also collects all errors, but raises an exception for the first validation error in the errors list; this was to keep the new functionality close to how it works now.

Implementation Notes:

econchick commented 8 years ago

Hey @cerivera - super sorry for the delay! I was traveling when you submitted this, but I'm back now and will review/comment today/tomorrow. Thanks for your patience!

econchick commented 8 years ago

Hey @cerivera!

So I think here may be a better way to do this.

First, there's no need to have an empty return statement within the validators.py. Python will automatically do that for you.

I'm wondering if rather than rewrite all the validators.py code to "collect" the errors in a list, that there could be a decorator to collect everything. Something like:

@collecterrors
def root_version(inst, attr, value):
    ...

What do you think?

cerivera commented 8 years ago

I like that decorator idea a lot! I'll work on it.

econchick commented 8 years ago

:+1: thanks so much!

econchick commented 8 years ago

Hey @cerivera - any update on this PR? Please ping me here if you need a review.

cerivera commented 8 years ago

@econchick I plan on getting to it this weekend; i got pulled into some stuff right after our last conversation.

econchick commented 8 years ago

@Cerivera awesome! Not a problem and don't mean to rush you at all. It's just a really great feature that I'm excited to have a contributor for :) let me know if I can help in any way!

cerivera commented 8 years ago

@econchick Fixes are in!

Here's the new usage (copied from usage.rst):

>>> from ramlfications import validate
>>> RAML_FILE = "/path/to/my-api.raml"
>>> validate(RAML_FILE)
>>>

>>> from ramlfications import validate
>>> RAML_FILE = "/path/to/invalid/no-title.raml"
>>> validate(RAML_FILE)
InvalidRootNodeError: RAML File does not define an API title.

>>> from ramlfications import validate
>>> RAML_FILE = "/path/to/invalid/no-base-uri-no-title.raml"
>>> validate(RAML_FILE)
MultipleValidationErrors:
InvalidRootNodeError: RAML File does not define the baseUri.
InvalidRootNodeError: RAML File does not define an API title.
econchick commented 8 years ago

Hey @cerivera ! Thanks so much!! I haven't looked at the code yet, but the usage you wrote looks like the exact behavior that I imagine.

I'm needing to make a flight home to Seattle for the week (family emergency) but I should have this reviewed in the next day or two.

econchick commented 8 years ago

hey @cerivera ! nice job - the code looks good to me! Just a couple of things that I already mentioned that I'd like your thoughts on. Also if you could squash the 20 commits into one or two commits (I like a clean git history), it would be awesome.

Thanks for the great work!

cerivera commented 8 years ago

@econchick thanks for the feedback! i agreed with all points and pushed a new commit that overwrote the rest.

econchick commented 8 years ago

Ok so pardon my stupid comments/questions this morning :)

Everything looks good! Merging now - will include this in the 1.7 release which will go out in the next day or so.

cerivera commented 8 years ago

@econchick I replaced MultipleValidationErrors with InvalidRAMLError and tabbed the errors in the __str__ method.

econchick commented 8 years ago

@cerivera if you're ever in SF, let me buy you a beer :)

Thanks for everything - this looks really good to me. As I said before - will include this in the next release (1.7) in the next couple of days (just want to wait for #40 )

cerivera commented 8 years ago

@econchick woohoo! I learned a ton working on your project, and I do like beer :)

ciao!