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.

Sign up for GitHub

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

Jump to bottom

Validator improvements #515

Merged
merged 3 commits into from
Oct 27, 2020
Merged

Validator improvements #515

merged 3 commits into from
Oct 27, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 23, 2020 •
edited
Loading

Closes #514, supercedes #513

Blocked by #503.

This PR fixes the above problems, and tidies some of the @test_case decorator into the ValidatorResults class.

  • Tidy up ValidatorResults & test_case
  • Ask for all supported response fields and validate them when doing chosen_entry query (closes 'Chosen entry had no value for ...' when property is not requested #514)
  • Better error-handling for case where no archetypal entry could be found (e.g. endpoint is empty)
  • Use response fields when doing multi/single entry endpoint tests to remove some deserialization errors
  • Temporary fix for response_fields and unset values: turn off exclude_unset when handling query params. This will need a proper treatment based on the actual requested fields, pending Responses for unknown fields #504...

@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch 3 times, most recently from 7acf4c4 to 6020edc Compare September 23, 2020 14:05
@ml-evs ml-evs mentioned this pull request Sep 23, 2020
Merged
1 task
@ml-evs ml-evs added blocked For issues/PRs that are blocked by required changes/clarifications to the specification. validator Related to the OPTIMADE validator labels Sep 23, 2020
@ml-evs ml-evs changed the title [BLOCKED] Validator improvements Validator improvements Sep 23, 2020
@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch from fbcfd4a to b5a073e Compare October 19, 2020 15:17
@codecov
Copy link

codecov bot commented Oct 19, 2020 •
edited
Loading

Codecov Report

Merging #515 into master will increase coverage by 0.15%.
The diff coverage is 89.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
+ Coverage   91.51%   91.67%   +0.15%     
==========================================
  Files          62       62              
  Lines        3135     3182      +47     
==========================================
+ Hits         2869     2917      +48     
+ Misses        266      265       -1
Flag Coverage Δ
#project 91.67% <89.34%> (+0.15%) ⬆️
#validator 64.51% <68.03%> (+1.10%) ⬆️

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

Impacted Files Coverage Δ
optimade/validator/validator.py 82.18% <88.33%> (-0.28%) ⬇️
optimade/validator/utils.py 94.63% <89.28%> (+4.33%) ⬆️
optimade/server/routers/utils.py 98.52% <100.00%> (+1.53%) ⬆️
optimade/validator/config.py 100.00% <100.00%> (ø)

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 fbbad72...9681f86. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch from b5a073e to 4ede5b2 Compare October 19, 2020 15:24
@ml-evs ml-evs removed the blocked For issues/PRs that are blocked by required changes/clarifications to the specification. label Oct 19, 2020
@ml-evs ml-evs mentioned this pull request Oct 19, 2020
Closed
@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch 3 times, most recently from 2f3c1a4 to 5615abc Compare October 19, 2020 21:57
@ml-evs ml-evs requested a review from CasperWA October 19, 2020 22:03
@ml-evs ml-evs added the priority/medium Issue or PR with a consensus of medium priority label Oct 19, 2020
@ml-evs ml-evs requested a review from shyamd October 26, 2020 09:48
shyamd
shyamd previously approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@shyamd shyamd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise this looks good.

optimade/server/routers/utils.py
Comment on lines +85 to +88
new_entry = entry.dict(exclude_unset=False)
for field in ("relationships", "links", "meta", "type", "id"):
if field in new_entry and new_entry[field] is None:
del new_entry[field]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use the exclude_none parameter here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not: the problem is that we want to include None values from all entry properties, but not from these top-level response fields. I'm sure there is a neater way of doing this, but I'd rather be explicit for now as its hopefully only a temporary "fix" before it can be dealt with properly in #504.

@ml-evs
Copy link
Member Author

ml-evs commented Oct 26, 2020

I'll give @CasperWA until tomorrow to have a look if he wants to, if not I'll merge in the morning.

CasperWA
CasperWA requested changes Oct 27, 2020
View reviewed changes
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.

Cheers @ml-evs !

I'm having a bit of trouble penetrating all of your changes here, but it seems all right.
I've put in a few comments, nothing serious I believe.

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

ml-evs commented Oct 27, 2020

I'm having a bit of trouble penetrating all of your changes here, but it seems all right.

Yeah, this one got a bit out of hand, but think it would have been more annoying as multiple PRs. I'll rebase this one into a few feature commits to maintain some kind of structure.

@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch from fc63615 to a70b0d2 Compare October 27, 2020 13:25
@ml-evs
Use response fields in validation (closes #514)
9681f86 
- when getting a "chosen entry"
- when making a request of SHOULD/OPTIONAL fields of entry endpoints
- improved error handling of chosen_entry and empty endpoints
- Until None valued response fields are applied based on the
entry schema and the response fields query parameter, return
all unset fields with exclude_unset=False when handling response
fields (should be dealt with in #504)
@ml-evs ml-evs force-pushed the ml-evs/validator_improvements branch from a70b0d2 to 9681f86 Compare October 27, 2020 13:26
@ml-evs ml-evs requested a review from CasperWA October 27, 2020 13:49
CasperWA
CasperWA approved these changes Oct 27, 2020
View reviewed changes
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.

Cheers @ml-evs 👍

@ml-evs ml-evs merged commit 6f34399 into master Oct 27, 2020
@CasperWA CasperWA deleted the ml-evs/validator_improvements branch October 27, 2020 14:45
@ml-evs ml-evs added the enhancement New feature or request label Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CasperWA CasperWA CasperWA approved these changes

@shyamd shyamd shyamd left review comments

Assignees
No one assigned
Labels
enhancement New feature or request priority/medium Issue or PR with a consensus of medium priority validator Related to the OPTIMADE validator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

'Chosen entry had no value for ...' when property is not requested
3 participants
@ml-evs @shyamd @CasperWA