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

Seperated Links from JSON API into its own file #30

Merged
merged 7 commits into from Jun 14, 2019
Merged

Conversation

tpurcell90
Copy link
Contributor

Links was modified by the Optimade API from the standard JSON API. I moved it to a different file
to reflect this change

Links was modified by the Optimade API from the standard JSON API. I moved it to a different file
to reflect this change
@tpurcell90 tpurcell90 requested a review from ltalirz June 13, 2019 15:42
@tpurcell90 tpurcell90 self-assigned this Jun 13, 2019
I added the base Links class because it is needed in JSONApi. I just define next, since pydantic
does not allow additionalProperties
Copy link
Contributor

@fawzi fawzi left a comment

Choose a reason for hiding this comment

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

I dislike the circular dependency links.py need jsonapi.py for Link which requires links.py for Links.
I would keep the old status.

@tpurcell90
Copy link
Contributor Author

I dislike the circular dependency links.py need jsonapi.py for Link which requires links.py for Links.
I would keep the old status.

I was in the process of changing that yesterday before dinner

Removed ciricular refrences and remade an attributes definition to follow
optimade API's definitions
"""References to other resource objects in a to-one (\"relationship\"). Relationships can be specified by including a member in a resource's links object."""

class RelationshipToMany(Set[Linkage]):
"""An array of objects each containing \"type\" and \"id\" members for to-many relationships."""
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 it necessary to escape quotation marks here?
Does it otherwise break the JSON?
I would have thought pydantic could be a bit more clever...

meta: Optional[dict]
links: Optional[ErrorLinks]

class Links(Links):
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this very ugly, redefinition of the same name when importing something? Looks like a problem waiting to happen

Copy link
Member

Choose a reason for hiding this comment

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

you could simply from .jsonapi import Links as JSONLinks

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference here would be to just import the namespace and then use jsonapi.Links as the base class.

Copy link
Member

Choose a reason for hiding this comment

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

even better!

Added missing file changes, and did a minor refactor on Error
Also removed Realtionships as it is not valid
…ade-python-tools into links_seperated

Manually fixed merge errors on my local machine
Copy link
Contributor

@dwinston dwinston left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@fawzi fawzi left a comment

Choose a reason for hiding this comment

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

great, thanks

@fawzi fawzi merged commit 6c0bbc2 into master Jun 14, 2019
@CasperWA CasperWA deleted the links_seperated branch November 8, 2019 14:36
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

4 participants