Propose v0.0.2 Issuer HTTP API

w3c-ccg / vc-issuer-http-api

A specification for an HTTP API used to issue Verifiable Credentials.

https://w3c-ccg.github.io/vc-http-api/

Other

7 stars 5 forks source link

Propose v0.0.2 Issuer HTTP API #30

Closed OR13 closed 4 years ago

OR13 commented 4 years ago

Expect breaking changes in v1... This PR is to unblock interop.

OR13 commented 4 years ago

I'm going to resolve all out dated comments so we can focus debate on remaining items, and I will be re-requesting reviews.

OR13 commented 4 years ago

I have merged the suggestions PR and resolved / the current feedback / comments. Please review again.

msporny commented 4 years ago

I'm going to resolve all out dated comments

Please don't do this... people miss topics that were discussed when you do this. Just because you and I agree on a topic doesn't mean the other reviewers are going to agree, and it's helpful to show others that a particular discussion happened and resulted in a change to the document or decision made.

OR13 commented 4 years ago

@msporny We cannot focus comments on where changes are requested if we leave all the conversations open... and it encourages endless debate with no clear action.

You should leave comments and request changes, or approve.

If you feel I have resolved something that does not have sufficient consensus, you should open a new comment and request changes.

peacekeeper commented 4 years ago

@OR13 I don't think it's good style to merge someone's PR and then immediately overwrite much of it, e.g.:

You changed create to compose in many places, but not everywhere, creating inconsistency (e.g. https://github.com/w3c-ccg/vc-issuer-http-api/pull/30#pullrequestreview-373685208). I prefer to keep "create" and not change that.
You changed the endpoint /credentials/issue to /credentials. My intention was to have /credentials/issue and /credentials/createAndIssue, to clearly express the difference.
You actually broke the API in 8c1c414cf2df1a415f254e612f9ae120ffc978f1, it's not valid right now.

Please revert those commits you did on top of my PR, those should be discussed separately.

OR13 commented 4 years ago

@peacekeeper my bad, I clearly did not handle the concept of a PR against a PR, with suggested changes from multiple parties correctly.

OR13 commented 4 years ago

@peacekeeper @msporny I am attempting to split the difference between your perspectives regarding HTTP verbs / naming... I expect you will both be disappointed.

With the understanding that nobody is going to be implementing this API but us, and the knowledge that we are very likely to redesign all of this, please review again.

If you can make comments here, instead of PRs against my fork, it will help the conversation stay in 1 place, and me with my terrible git skills.

OR13 commented 4 years ago

This PR has approvals from all CODEOWNERS, and couple other folks, any objection to merging?

OR13 commented 4 years ago

Feels like a lot of the debates on PRs are because we never got consensus on an API Style Guide: https://github.com/w3c-ccg/vc-issuer-http-api/issues/41