Skip to content
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

Merged
merged 3 commits into from Sep 17, 2020
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 15, 2020

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.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #502 into master will increase coverage by 0.00%.
The diff coverage is 97.82%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #502   +/-   ##
=======================================
  Coverage   91.58%   91.59%           
=======================================
  Files          61       61           
  Lines        3103     3118   +15     
=======================================
+ Hits         2842     2856   +14     
- Misses        261      262    +1     
Flag Coverage Δ
#project 91.59% <97.82%> (+<0.01%) ⬆️
#validator 63.72% <69.56%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/validator/__init__.py 9.09% <0.00%> (-0.29%) ⬇️
optimade/validator/utils.py 90.29% <100.00%> (+0.14%) ⬆️
optimade/validator/validator.py 82.67% <100.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e05e0b8...698a424. Read the comment docs.

Copy link
Member

@CasperWA CasperWA left a 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.

optimade/validator/validator.py Outdated Show resolved Hide resolved
tests/server/test_server_validation.py Outdated Show resolved Hide resolved
tests/server/test_server_validation.py Show resolved Hide resolved
@ml-evs
Copy link
Member Author

ml-evs commented Sep 16, 2020

Now we have the same problem as with the other PR, mkdocs can't handle dataclasses or NamedTuple :(

@CasperWA
Copy link
Member

Now we have the same problem as with the other PR, mkdocs can't handle dataclasses or NamedTuple :(

I wonder if there is a comment syntax (like # pylint: disable=some-rule) to exclude a class from MkDocs?
Otherwise, we should contribute to MkDocs, or rather its doc-string parser 😕

@CasperWA
Copy link
Member

Now we have the same problem as with the other PR, mkdocs can't handle dataclasses or NamedTuple :(

I wonder if there is a comment syntax (like # pylint: disable=some-rule) to exclude a class from MkDocs?
Otherwise, we should contribute to MkDocs, or rather its doc-string parser confused

Looking into the error from the CI run, it seems it is not MkDocs, but rather mkdocstrings.

@ml-evs
Copy link
Member Author

ml-evs commented Sep 16, 2020

Now we have the same problem as with the other PR, mkdocs can't handle dataclasses or NamedTuple :(

I wonder if there is a comment syntax (like # pylint: disable=some-rule) to exclude a class from MkDocs?
Otherwise, we should contribute to MkDocs, or rather its doc-string parser confused

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 api_reference/validator/validator.md 😕

@CasperWA
Copy link
Member

CasperWA commented Sep 16, 2020

Now we have the same problem as with the other PR, mkdocs can't handle dataclasses or NamedTuple :(

I wonder if there is a comment syntax (like # pylint: disable=some-rule) to exclude a class from MkDocs?
Otherwise, we should contribute to MkDocs, or rather its doc-string parser confused

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 api_reference/validator/validator.md confused

So what it's doing (I'm guessing) is following the reference (to optimade/validator/validator.py) and using pytkdocs to extract the docstrings, then it's converting this to XML, which is where it fails, because the generated XML file is badly made.

@ml-evs
Copy link
Member Author

ml-evs commented Sep 16, 2020

So what it's doing (I'm guessing) is following the reference (to optimade/validator/validator.py) and using pytkdocs to extract the docstrings, then it's converting this to XML, which is where it fails, because the generated XML file is badly made.

So it "works" if I remove the explicit dataclass.field fields, though it still returns warnings for __eq__, __init__ and __repr__ (as is the case with NamedTuple in the other PR) :(

Copy link
Member

@CasperWA CasperWA left a 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
Copy link
Member

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
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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

@ml-evs ml-evs merged commit 56a4d0c into master Sep 17, 2020
@CasperWA CasperWA deleted the ml-evs/validator_json branch September 17, 2020 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants