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

Change default MIME type to "application/vnd.api+json" #877

Merged
merged 6 commits into from
Jul 15, 2021
Merged

Change default MIME type to "application/vnd.api+json" #877

merged 6 commits into from
Jul 15, 2021

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jul 14, 2021

Closes #875, so that we can adhere to the JSON:API spec on content negotiation. This also has an effect on the OpenAPI schemas.

Also:

  • made some tweaks to our exception definitions, removing the old Mixin class and replacing it with a new base class.
  • use the specific exceptions everywhere rather than the generic HTTPException.

@codecov
Copy link

codecov bot commented Jul 14, 2021 •
edited
Loading

Codecov Report

Merging #877 (c9975aa) into master (90ad30b) will increase coverage by 0.06%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   92.67%   92.74%   +0.06%     
==========================================
  Files          67       67              
  Lines        3768     3763       -5     
==========================================
- Hits         3492     3490       -2     
+ Misses        276      273       -3
Flag Coverage Δ
project 92.74% <93.54%> (+0.06%) ⬆️
validator 92.74% <93.54%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
optimade/server/routers/utils.py 98.55% <83.33%> (+0.03%) ⬆️
optimade/server/exceptions.py 97.05% <94.11%> (+6.81%) ⬆️
optimade/server/exception_handlers.py 84.28% <100.00%> (-0.23%) ⬇️
optimade/server/main.py 94.33% <100.00%> (ø)
optimade/server/main_index.py 94.73% <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 90ad30b...c9975aa. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/change_default_media_type branch from 4887d3d to 52d4f4f Compare July 14, 2021 11:31
CasperWA
CasperWA requested changes Jul 14, 2021
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.

Looks good. Only very minor things.

optimade/server/exceptions.py Outdated
Comment on lines 25 to 38
def __init__(self, detail: str = None, headers: dict = None) -> None:
super().__init__(status_code=self.status_code, detail=detail, headers=headers)
Copy link
Member

Choose a reason for hiding this comment

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

You should set a standard status_code, e.g., 500?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe just define it and set it to None?

Copy link
Member

Choose a reason for hiding this comment

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

Or is it already defined by the parent class?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was toying with being a bit more defensive here, so happy to add it

Copy link
Member

Choose a reason for hiding this comment

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

Well the issue is that it's referring to a parameter it doesn't have?

optimade/server/routers/utils.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/change_default_media_type branch from 449c790 to 06f45be Compare July 14, 2021 15:35
@ml-evs ml-evs requested a review from CasperWA July 14, 2021 15:35
@ml-evs
Copy link
Member Author

ml-evs commented Jul 14, 2021

Wondering if merging this into the spec repo will cause any issues... of course we implicitly had this constraint via JSON:API, but no-one is following it

JPBergsma
JPBergsma reviewed Jul 14, 2021
View reviewed changes
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

Good work all that repeated code in "exceptions.py" was begging for a rewrite.

CasperWA
CasperWA previously approved these changes Jul 15, 2021
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.

Minor thing, it's essentially good to go.

optimade/server/exceptions.py Outdated Show resolved Hide resolved
@ml-evs @CasperWA
Add missing __str__ type hint
c9975aa 
Co-authored-by: Casper Welzel Andersen <[email protected]>
@ml-evs ml-evs requested a review from CasperWA July 15, 2021 15:18
@ml-evs
Copy link
Member Author

ml-evs commented Jul 15, 2021

Won't go through another review... will merge here once the tests pass

@ml-evs ml-evs merged commit e4c07cc into master Jul 15, 2021
@ml-evs ml-evs deleted the ml-evs/change_default_media_type branch July 15, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JPBergsma JPBergsma JPBergsma left review comments

@CasperWA CasperWA Awaiting requested review from CasperWA

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change MIME type to application/vnd.api+json where appropriate
3 participants
@ml-evs @JPBergsma @CasperWA