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

Gizmo JSON-LD Content Type #821

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Gizmo JSON-LD Content Type #821

merged 2 commits into from
Sep 25, 2019

Conversation

iddan
Copy link
Collaborator

@iddan iddan commented Sep 15, 2019

This change is Reviewable

Todo

  • Fix tests

Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

The change looks great, thanks @iddan! :)

However it will break all clients. As with the previous PR, we can add it in a compatible manner. I think we can switch on Accept header of incoming requests and serve the new format specifically for Accept: application/ld+json. We can then land this change to v0.7.6 and switch to JSON-LD by default in v0.8.

What do you think?

@iddan
Copy link
Collaborator Author

iddan commented Sep 16, 2019

It will be awesome! Can you help me to do that?

@dennwc dennwc self-assigned this Sep 16, 2019
@dennwc
Copy link
Member

dennwc commented Sep 16, 2019

Sure, will try to push changes to the same branch.

@dennwc
Copy link
Member

dennwc commented Sep 23, 2019

@iddan I rebased your branch and exposed the new code path when Accept: application/ld+json is set in HTTP headers. See Gizmo's Session.quadValueToNative for details.

Can you please fix tests and check if HTTP endpoint returns what we expect?

I used this command to test it:

curl -H 'Accept: application/ld+json' 'http://localhost:64210/api/v2/query?lang=gizmo&qu=g.V("<bob>").all()'

@iddan
Copy link
Collaborator Author

iddan commented Sep 23, 2019

Sure, will do it soon.

@iddan
Copy link
Collaborator Author

iddan commented Sep 23, 2019

@dennwc tests are fixed, the response was checked

@iddan iddan changed the title Gizmo to return JSON-LD Gizmo JSON-LD Content Type Sep 24, 2019
@iddan iddan added this to the v0.7.6 milestone Sep 24, 2019
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Applied some Git magic to rebase/squash unnecessary commits, ready to merge now. Thanks again @iddan! :)

@dennwc dennwc merged commit 74330c2 into cayleygraph:master Sep 25, 2019
@iddan iddan deleted the jsonld-results branch September 26, 2019 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants