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

Add persisted id to payload #16

Merged
merged 5 commits into from
Dec 8, 2016
Merged

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Oct 5, 2016

Fixes #15.

Blocks #5.

(work, '../work/{}', 'work')]:
ld_data = entity.to_jsonld()
ld_data['@id'] = id_template.format(entity.persist_id)
res[key] = ld_data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is somewhat messy, since we have to use the relative URL here if we want to be JSON-LD compliant (related #14). It's not as nice as having just the ID, but stripping the path to get the ID is easy enough and I prefer this over a few alternatives on where to put the ID.

@sohkai sohkai force-pushed the feat/15/add-persisted-id-to-payload branch from b7dbb8a to 202d26f Compare October 5, 2016 13:31
@sohkai
Copy link
Contributor Author

sohkai commented Oct 5, 2016

Also part of the fix for COALAIP/pycoalaip#45.

Copy link
Contributor

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

(btw. I chose to try out the feedback feature of Github this time)

Lets discuss why we're not linking to the transaction in bigchaindb.


In the case of the returned Work and Copyright, their `@id`s are slightly
inconvenient to process as they live under a different base URL (`/work` and
`/right`, respectively). You should strip awsay the leading pathes to use just
Copy link
Contributor

Choose a reason for hiding this comment

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

away

assert bool(manifestation['manifestationOfWork']) is True

# Check @ids
assert copyright_['@id'].startswith('../right/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I'm misunderstanding this but shouldn't this link relatively to another bigchaindb transaction instead of .../right/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; the relative link is based on the URL of the current request, which in this case is <coala-http-api>/api/v1/manifestation/<id>

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Needs to be /rights/

@TimDaub TimDaub force-pushed the feat/15/add-persisted-id-to-payload branch from 2de2ccb to 935ab51 Compare December 8, 2016 09:55
@TimDaub TimDaub merged commit 945facf into master Dec 8, 2016
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.

2 participants