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

Validator changes: always check unversioned '/versions' and handle rich HTML pages #665

Merged
merged 3 commits into from Jan 8, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jan 6, 2021

This PR does two things:

  • Some services e.g. netlify have rich 404 pages which yield unhelpful error messages, those error messages are now improved.
  • Following discussion in Implement version negotiation for validator #662, this PR always checks the existence of the unversioned /versions endpoint, even when passed a versioned URL

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #665 (4261f3e) into master (33de78f) will decrease coverage by 0.14%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
- Coverage   93.45%   93.30%   -0.15%     
==========================================
  Files          61       61              
  Lines        3299     3300       +1     
==========================================
- Hits         3083     3079       -4     
- Misses        216      221       +5     
Flag Coverage Δ
project 93.30% <25.00%> (-0.15%) ⬇️
validator 66.06% <16.66%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
optimade/validator/validator.py 82.22% <18.18%> (-0.93%) ⬇️
optimade/validator/utils.py 94.52% <100.00%> (-0.12%) ⬇️

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 33de78f...4261f3e. Read the comment docs.

@ml-evs ml-evs added bug Something isn't working priority/high Issue or PR with a consensus of high priority validator Related to the OPTIMADE validator labels Jan 6, 2021
@ml-evs ml-evs requested a review from CasperWA January 6, 2021 15:40
optimade/validator/validator.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/validator_rich_page_fix branch from ccb9cb5 to db5fc58 Compare January 7, 2021 14:08
@ml-evs ml-evs force-pushed the ml-evs/validator_rich_page_fix branch from db5fc58 to aad6b5f Compare January 7, 2021 14:28
@ml-evs ml-evs added priority/low Issue or PR with a consensus of low priority and removed priority/high Issue or PR with a consensus of high priority labels Jan 7, 2021
@ml-evs
Copy link
Member Author

ml-evs commented Jan 7, 2021

Changed to low-priority as this is only an issue in cases where the validator should fail anyway, i.e. this PR is just an error message improvement, initially we thought it was bug fix that it was failing because of this

@ml-evs ml-evs changed the title Fix for rich HTML error pages that cannot be JSON decoded Tweak error message for rich HTML error pages that cannot be decoded as JSON Jan 7, 2021
@ml-evs ml-evs requested a review from CasperWA January 7, 2021 14:41
@ml-evs ml-evs changed the title Tweak error message for rich HTML error pages that cannot be decoded as JSON Validator changes: always check unversioned '/versions' and handle rich HTML pages Jan 7, 2021
@ml-evs ml-evs force-pushed the ml-evs/validator_rich_page_fix branch from ae01e5f to 1bd20b9 Compare January 7, 2021 17:04
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.

Looks good to me, cheers @ml-evs !

@ml-evs ml-evs merged commit 8fa2200 into master Jan 8, 2021
@ml-evs ml-evs deleted the ml-evs/validator_rich_page_fix branch January 8, 2021 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority/low Issue or PR with a consensus of low priority validator Related to the OPTIMADE validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants