Complete editorial overhaul of usubscription.proto#323
Complete editorial overhaul of usubscription.proto#323AnotherDaniel wants to merge 1 commit intoeclipse-uprotocol:mainfrom
Conversation
sophokles73
left a comment
sophokles73
left a comment
There was a problem hiding this comment.
In general, I do not think that it is a good idea to put all the effort for documenting the service interface into the proto3 file. FMPOV we should define the service in the specification document using appropriate means, e.g. UML and textual description.
The proto3 file is simply a representation of it that can be fed into a protoc compiler...
| // Register the calling client as subscriber to a topic, passing a SubscriptionRequest | ||
| // message containing the desired topic and optional SubscriptionAttributes. | ||
| // * Returns a SubscriptionResponse message containing the status of the request, | ||
| // along with any event delivery configuration required to consume the event. | ||
| // * Calling this endpoint also registers the subscribing client to receive subscription | ||
| // change notifications when the subscription state changes. |
There was a problem hiding this comment.
most of this is already clear from the operation definition, i.e. which message goes in and which one is returned. IMHO the docs for these message types should then be enough to describe the contained params ...
| // Register the calling client as subscriber to a topic, passing a SubscriptionRequest | |
| // message containing the desired topic and optional SubscriptionAttributes. | |
| // * Returns a SubscriptionResponse message containing the status of the request, | |
| // along with any event delivery configuration required to consume the event. | |
| // * Calling this endpoint also registers the subscribing client to receive subscription | |
| // change notifications when the subscription state changes. | |
| // Registers the calling client as subscriber to a topic. | |
| // Calling this operation also registers the subscribing client to receive subscription | |
| // change notifications when the subscription state changes. |
| // Unregister the calling susbcriber client from a topic. | ||
| // * Returns a UnsubscripeResponse message containing the updated status of the subscription. | ||
| // * Calling this endpoint also unregisters the client from receiving subscription change | ||
| // notifications for this subscription. |
There was a problem hiding this comment.
| // Unregister the calling susbcriber client from a topic. | |
| // * Returns a UnsubscripeResponse message containing the updated status of the subscription. | |
| // * Calling this endpoint also unregisters the client from receiving subscription change | |
| // notifications for this subscription. | |
| // Unsubscribes the calling client from a topic. | |
| // Also unregisters the client from receiving subscription change | |
| // notifications for this subscription. |
| } | ||
|
|
||
| // Fetch a list of subscriptions | ||
| // Fetch a list of subscribers that are currently subscribed to a given topic. |
There was a problem hiding this comment.
| // Fetch a list of subscribers that are currently subscribed to a given topic. | |
| // Fetches a list of subscribers that are currently subscribed to a given topic. |
| option (uprotocol.method_id) = 8; | ||
| } | ||
|
|
||
| // Fetch a list of subscriptions for a given subscriber or topic. |
There was a problem hiding this comment.
| // Fetch a list of subscriptions for a given subscriber or topic. | |
| // Fetches a list of subscriptions for a given subscriber or topic. |
|
|
||
| // Register to receive subscription change notifications that are published on the | ||
| // 'up:/core.usubscription/3/subscriptions#Update' | ||
| // Register client to receive subscription change notifications for a given topic. |
There was a problem hiding this comment.
| // Register client to receive subscription change notifications for a given topic. | |
| // Registers the caller to receive subscription change notifications for a given topic. |
| } | ||
|
|
||
| // Unregister for subscription change events | ||
| // Unregister client from receiving subscription change events for a given topic. |
There was a problem hiding this comment.
| // Unregister client from receiving subscription change events for a given topic. | |
| // Unregisters the caller from receiving subscription change events for a given topic. |
| // reset). | ||
| // **__NOTE:__** This is a private API only for uSubscription services, | ||
| // uEntities can call Unsubscribe() to flush their own subscriptions. | ||
| // Reset all subscriber-topic registrations of a uSubscription Service. |
There was a problem hiding this comment.
| // Reset all subscriber-topic registrations of a uSubscription Service. | |
| // Clears all subscription state held by the called uSubscription service instance. |
I was thinking that too - but this means we'd simply remove all the docs from the proto file (and backsync with the textual spec where necessary). Should we? (remove the docs from proto) |
IMHO we shouldn't remove all comments altogether but instead strip them down to the bare minimum, adding pointers to the specification document for reference. @PLeVasseur @stevenhartley WDYT? |
|
I was wondering whether to simply use requirement links back to the textual spec... |
You mean OpenFastTrace spec items? |
|
Yes, I was thinking OFT dsn items, linking back to the corresponding req items in the text. That way, the text spec formally becomes the source of truth, and can be implemented/dsn-detailed by any specific technology in the future (should we ever move away from proto, for instance). |
2 participants
Update, improve and cleanup comments and documentation of usubscription.proto
This is a complete once-over of almost every doc-string in the usubscription protobuf definition, and the first part of the work to address #321