-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable JSON response from the validator #502
Conversation
fa6ef23
to
d0556bd
Compare
Codecov Report
@@ Coverage Diff @@
## master #502 +/- ##
=======================================
Coverage 91.58% 91.59%
=======================================
Files 61 61
Lines 3103 3118 +15
=======================================
+ Hits 2842 2856 +14
- Misses 261 262 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically a sale for me - I just review as a "Comment" to make sure my few comments are indeed considered :)
Mainly, if you create a class for the results
attribute instead of a simple dict
you could still use the old writing way of retrieving the various results, i.e., using .
(dot) instead of key access.
Now we have the same problem as with the other PR, mkdocs can't handle |
I wonder if there is a comment syntax (like |
Looking into the error from the CI run, it seems it is not MkDocs, but rather mkdocstrings. |
Yeah sorry, that's what I mean every time I say mkdocs... this one seems weird, mkdocstrings is meant to support dataclasses and the error is to do with parsing the markdown |
So what it's doing (I'm guessing) is following the reference (to |
So it "works" if I remove the explicit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very kewl @ml-evs
Approved, and a single question :)
@@ -242,19 +241,20 @@ def wrapper( | |||
if not isinstance(result, Exception): | |||
if not multistage: | |||
if not optional: | |||
validator.success_count += 1 | |||
validator.results.success_count += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OooOooohh ✨
__all__ = ("ImplementationValidator",) | ||
|
||
|
||
@dataclasses.dataclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: Is there a reason you're not using pydantic here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, guess this is slightly more lightweight but you could use either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just wondering if this would solve the documentation issue as well...
This PR adds a
-j
/--json
flag to the validator that lowers verbosity to "-1" and only prints a JSON representation of the results to stdout.