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

Describe N3 Patch #346

Merged
merged 30 commits into from
Dec 15, 2021
Merged

Describe N3 Patch #346

merged 30 commits into from
Dec 15, 2021

Conversation

RubenVerborgh
Copy link
Contributor

Closes #332

Copy link
Contributor Author

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

This is the initially proposed text; if approved, I can also add an example.

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
@RubenVerborgh RubenVerborgh added the category: revisit for 1.0 Temporary resolution achieved for 0.9, but might want to reconsider for 1.0. label Nov 10, 2021
@RubenVerborgh

This comment has been minimized.

@RubenVerborgh RubenVerborgh marked this pull request as ready for review November 10, 2021 18:03
@RubenVerborgh
Copy link
Contributor Author

@csarven @justinwb @kjetilk @timbl Ready for review.

protocol.html Outdated Show resolved Hide resolved
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

I think this is going in a direction that will be workable very soon. My comments are mostly editorial, but I think needed to get into line with the language of the rest of the spec.

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

One more thing, it should mention what kind of operations these are (i.e. append, read, write), so that authz systems can hook into that, also in line with #220 . I guess you can adopt some of the language I used for this #320.

Copy link
Member

@justinwb justinwb left a comment

Choose a reason for hiding this comment

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

Overall looks good - just a few comments

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
@RubenVerborgh
Copy link
Contributor Author

One more thing, it should mention what kind of operations these are (i.e. append, read, write), so that authz systems can hook into that, also in line with #220 . I guess you can adopt some of the language I used for this #320.

In 76398fe

@RubenVerborgh
Copy link
Contributor Author

Rebased; requested change in cd44b69 /CC @timbl

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
@RubenVerborgh
Copy link
Contributor Author

@csarven Thanks, amended 2a99d6a

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

OK, great! I think we should think economy when requiring new triples, but indeed, the extensibility that this affords is important.

@kjetilk kjetilk linked an issue Dec 14, 2021 that may be closed by this pull request
@csarven csarven merged commit bcb778b into main Dec 15, 2021
@csarven
Copy link
Member

csarven commented Dec 19, 2021

dokieli implements clients side of N3 Patch: dokieli/dokieli@f4c1a0a

Currently used for patching a Memento TimeMap resource:

timemap-n3-patch

@csarven csarven deleted the feature/n3-patch branch May 12, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: new functionality Concerns a new feature category: revisit for 1.0 Temporary resolution achieved for 0.9, but might want to reconsider for 1.0. doc: Protocol topic: resource access
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Alternative update support with N3 patch Is Read required on top of Write for PATCH delete and where ?
6 participants