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.

Sign up for GitHub

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

Jump to bottom

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

@RubenVerborgh RubenVerborgh marked this pull request as draft November 10, 2021 11:40
@RubenVerborgh RubenVerborgh self-assigned this Nov 10, 2021
@RubenVerborgh RubenVerborgh added this to the Release 0.9 milestone Nov 10, 2021
RubenVerborgh
RubenVerborgh commented Nov 10, 2021
View reviewed changes
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
kjetilk
kjetilk reviewed Nov 10, 2021
View reviewed changes
protocol.html Outdated Show resolved Hide resolved
kjetilk
kjetilk reviewed Nov 10, 2021
View reviewed changes
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.

Sign in to view
@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.

@RubenVerborgh RubenVerborgh mentioned this pull request Nov 10, 2021
Closed
TallTed
TallTed reviewed Nov 10, 2021
View reviewed changes
protocol.html Outdated Show resolved Hide resolved
kjetilk
kjetilk requested changes Nov 11, 2021
View reviewed changes
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
kjetilk
kjetilk requested changes Nov 12, 2021
View reviewed changes
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.

justinwb
justinwb reviewed Nov 12, 2021
View reviewed changes
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
TallTed
TallTed reviewed Nov 12, 2021
View reviewed changes
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

csarven
csarven reviewed Dec 14, 2021
View reviewed changes
protocol.html Outdated Show resolved Hide resolved
csarven
csarven reviewed Dec 14, 2021
View reviewed changes
protocol.html Outdated Show resolved Hide resolved
csarven
csarven reviewed Dec 14, 2021
View reviewed changes
protocol.html Outdated Show resolved Hide resolved
@RubenVerborgh
Copy link
Contributor Author

@csarven Thanks, amended 2a99d6a

kjetilk
kjetilk approved these changes Dec 14, 2021
View reviewed changes
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
Is Read required on top of Write for PATCH delete and where ? #220
Closed
@csarven csarven merged commit bcb778b into main Dec 15, 2021
@csarven
Copy link
Member

csarven commented Dec 19, 2021 •
edited
Loading

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
Reviewers

@TallTed TallTed TallTed left review comments

@csarven csarven csarven approved these changes

@timbl timbl timbl approved these changes

@justinwb justinwb justinwb approved these changes

@kjetilk kjetilk kjetilk approved these changes

@ericprud ericprud Awaiting requested review from ericprud

Assignees

@kjetilk kjetilk

@csarven csarven

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
Specification
Status: Done
Milestone
Release 0.9
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
@RubenVerborgh @kjetilk @csarven @justinwb @timbl @TallTed