Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Change blanknodes to namednodes #181

Merged
merged 4 commits into from
Jan 16, 2020

Conversation

nlisgo
Copy link
Member

@nlisgo nlisgo commented Jan 14, 2020

The aim of this PR is to introduce the smallest amount of changes to swap out BlankNodes for NamedNodes.

@nlisgo
Copy link
Member Author

nlisgo commented Jan 15, 2020

@giorgiosironi I would like this to be considered for release. I know it doesn't address all of the todo items but it adds enough value in my opinion. The next piece of work will be to improve the identifier to include the /articles/ prefix and then we will want to allow the retrieval of a single article in the store.

I believe this will be enough to unblock the database persistence work.

@giorgiosironi giorgiosironi marked this pull request as ready for review January 15, 2020 12:08
@giorgiosironi giorgiosironi requested a review from a team as a code owner January 15, 2020 12:08
@@ -28,7 +28,7 @@ export default (): AppMiddleware => (
throw new createHttpError.BadRequest(`Article must have at least one ${termToString(schema('name'))}`);
}

const newId = blankNode(uniqueString());
const newId = namedNode(uniqueString());
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says this should be an IRI, but we have good reasons to use host-free values.

Copy link
Member

@giorgiosironi giorgiosironi left a comment

Choose a reason for hiding this comment

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

Checking NamedNode basic requirements I think this should go ahead.
The other tasks in libero/publisher#367 however still need to be followed up before persistence can be introduced as the whole ticket has been created as a blocker (also I see that at the bare minimum identifiers like /articles/... should be stable before they start to get persisted).

@giorgiosironi
Copy link
Member

Found out this couldn't be merged by @nlisgo because the branch protection on master limits pushing to Organization administrators, repository administrators, and users with the Maintain role plus Dependabot. https://github.com/orgs/libero/teams/node-js-reviewers only has Write, not Maintain permission.

@nlisgo nlisgo deleted the articles-dereferenceable branch January 17, 2020 16:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants