- Notifications
You must be signed in to change notification settings - Fork1.1k
Api notifications#153
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It might be better to use a URI template here: `/notifications{?all,participating,since}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah dang, I should make all the urls into templates. Thanks for reminding me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, do you prefer parameters or endpoints like/notifications/all and/notifications/participating? I guess they are technically separate collections.
content/v3/activity/notifications.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this mark all notifications a read? If not now is the appropriate notification identified? Wouldn'tDELETE be more appropriate than POST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this endpoint is for all notifications. There's also a per-repository URL: "/repos/:owner/:name/mark". Summaries have their own endpoint,/notifications/summaries/:id/mark.
I used POST instead of DELETE because you can still see the notifications if you pass?all=1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see.DELETE might be a little weird in that case. I don't really like those rpc style actions, though. I prefer the following
==>
PUT /notifications/summaries/:idcontent-type: app/v3{unread: false}<===
200 OKThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The double negative of the unread property in this case is a little awkward. Maybe this would be a better ux:
===> PUT /notifications/summaries/:id content-type: app/v3 {read: true}<---- 200 OKThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would this be applicable for collections too?
PUT /notifications{read: true}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thinkPUT would be a better method for this. Even if we don't want clients to have to send the full representation we should still usePUT. A PUT w/o a complete long form representation is not a "partial put" unless the server is unable to take that representation and convert it into the full representation using the existing state of the system in an idempotent manner. So PUTs with a range header are partial and not ok because if you repeat them they are not idempotent. PUTs with a "lite" representation of the resource arenot partial puts, even though that "lite" representation does not include all the resource information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, we use PATCH everywhere else. Had a big discussion internally and on Twitter back when I was designing the API. Seems like a difference in the vague specs are interpreted.
At least for now, I think going with PATCH for consistency is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
probably, i just hate patch so much fo this type of use case.
summary => threadremoved /mark rpc endpoint
…large-files-blog-postSuggested edits to Gist file truncation post
Trying something new here: spike out some documentation in public while we work on the implementation of the Notifications API.
[x]Add etag/last-modified support for polling[x]Show recent commenter + body[ ]Allow marking issues/commits/pulls as read, without having to fetch a thread's summary id.[x]Add some kind of type/ID to identify the thread type (commit vs pull vs issue).You can read theunprocessed docs, or pull this down and build them locally (
nanoc ; nanoc view ; open http://localhost:3000).