Here's the latest patch: http://codereview.appspot.com/33058
-Lane On Fri, Apr 3, 2009 at 1:32 PM, Scott Seely <[email protected]> wrote: > Line 603 | 611 | 624 | 639 | 666 : We should show a check for errors > prior to investigating result.[value]. People cut and paste this code all > the time. Error checking shouldn’t be confusing in the spec. It could be as > simple as adding > > if (!result.error){/*existing code*/} > good suggestion...added > > > Functions section (starting at 676): Each method that takes optional > parameters should have an <xref> to where the optional parameters are > defined. Also, do any examples show how to pass in the optional parameters > (I didn’t see any)? If not, please add examples. > I kept the simple examples and added some that show more optional parameters > > > Line 680: “Takes an optional. set" should be “Takes an optional set” > (delete the period after optional) > nice catch...fixed > > > > > > > *From:* [email protected] [mailto: > [email protected]] *On Behalf Of *Lane > LiaBraaten > *Sent:* Friday, April 03, 2009 1:16 PM > > *To:* [email protected] > *Cc:* [email protected] > *Subject:* [opensocial-and-gadgets-spec] Re: OS Lite (OSAPI) spec doc > > > > Here's the next pass: http://codereview.appspot.com/33058 > > On Fri, Apr 3, 2009 at 1:04 PM, Scott Seely <[email protected]> > wrote: > > Yes to the eref. > > > > I’m a bit concerned about the lack of parity between the JSON-RPC and osapi > for album, mediaitem, and messaging. Osapi and JSON-RPC are closely related. > > > > > I’m fine with leaving these undefined until v.next. Please post when you > are complete and I’ll give this another pass. > > > > *From:* [email protected] [mailto: > [email protected]] *On Behalf Of *Lane > LiaBraaten > *Sent:* Friday, April 03, 2009 11:52 AM > > > *To:* [email protected] > *Cc:* [email protected] > *Subject:* [opensocial-and-gadgets-spec] Re: OS Lite (OSAPI) spec doc > > > > Thanks for the review, Scott. I synced up with Bob to address the open > issues on this thread. I'm working on an updated patch, but in the mean > time, comments inline... > > -Lane > > On Fri, Apr 3, 2009 at 9:59 AM, Scott Seely <[email protected]> > wrote: > > Based on my review, this is still a work in progress. I do not guarantee > that this is a complete list of spec issues. These are simply the issues > that seem to be the most evident. > > > > Missing from spec: > > Messaging > > Albums > > MediaItem > > I think we should move these to v.next > > > > Line 588: “Highlights” is really a section. Please change from <t> to > <section title="Highlights"> > > fixed > > > > Line 600: Please strike. This is a sentence that occurs in the discussion > of why you included the examples early. It is not needed for the spec. > > removed > > > > Line 601: “A Person request, the most basic example” should be moved into > the <figure> tag as: > > <figure> > > <preamble>A basic Person Request</preamble> > > > > Line 609 & 618: Move content inside /figure/preamble tag > > fixed here and in a couple other places as well > > > > Line 631 Move this to Highlights list: Batch calls can be cascaded. > > Maybe I’m old school, but it seems that the correct term is > “chained|chaining”? Suggested edit when moving to Highlights (near line 588) > > “Batch calls are designed for call chaining.” > > The examples demonstrate this concept adequately. > > Moved to highlights list and changed to 'chaining'. > > > > Line 635. Please strike “This is a short namespace, so it's easier to type, > and it is different from the existing apis so that they can coexist until > the old apis are removed.” I don’t believe that this statement enhances the > spec. Line 586 already addresses deprecation. > > removed > > > > Line 648: “When the Gadget server renders the osapi library, it calls the > SystemHandler's list Methods endpoint, and adds the available services.” > > ‘it’ is ambiguous. > > SystemHandler is a new term. I believe that this may be a Shindig type, not > something that is actually part of the system. > > Line 648 contains a TODO: “The js client then uses this to generate (TODO > Not implemented yet)”. The document here MUST NOT refer to TODOs in any > given implementation. The document MUST NOT contain TODOs for feature > descriptions. I’m not sure which case the TODO refers to, but the TODO needs > to disappear and be rectified. > > removed implementation details (and TODO). The paragraph now reads: > > The features available on the client are determined by the server from > which the JavaScript is delivered. The server SHOULD generate service > methods for all available services. Developers wanting to write gadgets > safely can test for the existence of a service and method before making > their call. > > [example follows...] > > > > > > Line 661: Define behavior of <Require feature="osapi" /> > > > > Line 662 | 682 | 698 | 715 | 735 | 750 | 775 > > Why are we defining subsets of the feature? Osapi should be > a single feature, introducing the basic APIs around Activities, People, > AppData. The extra granularity seems to be new and unnecessary. I fail to > see the benefit of the extra knobs. > > Let's just use feature="osapi" for v0.9. We can revisit sub-features > later, but at this point it doesn't seem to provide much benefit. > > > > Line 662: “that map to the activities endpoint” should be “that map to the > people endpoint”. Need precise definition of what the endpoint is. Probably > need a cross reference to the spec where the endpoint for each item is > defined > > will do...this is an eref tag, right? > > > > Line 666 | 667 | 668 | 669: You repeat “convenience” 2x. Delete the ‘Note’ > > removed the duplication > > > > Line 682 | 698: Need precise definition of what the endpoint is. Probably > need a cross reference to the spec where the endpoint for each item is > defined > > will do. > > > > > > *From:* [email protected] [mailto: > [email protected]] *On Behalf Of *Lane > LiaBraaten > *Sent:* Thursday, April 02, 2009 9:45 PM > *To:* [email protected] > *Cc:* [email protected] > *Subject:* [opensocial-and-gadgets-spec] Re: OS Lite (OSAPI) spec doc > > > > oops - here's the patch: http://codereview.appspot.com/33058 > > On Thu, Apr 2, 2009 at 9:42 PM, Lane LiaBraaten <[email protected]> > wrote: > > I created a spec patch for the OSAPI stuff, but I didn't realize Bob hadn't > incorporated Adam's comments yet. I made a few changes, but some of these > we still need to finalize. My comments inline. > > -Lane > > On Tue, Mar 31, 2009 at 10:10 AM, Adam Winer <[email protected]> wrote: > > > Comments: > > - At this stage, we really need spec updates in the form of diffs to > > http://opensocial-resources.googlecode.com/svn/spec/draft/Opensocial-Specification.xml > . > I kinda wish we'd split this huuuge doc into multiple smaller docs, > as the XSLT transform hangs Firefox for awhile, but that should be its > own spec diff. > > - osapi.base should largely be undocumented, and I'm not sure it > should exist as a feature name, though I could be convinced otherwise. > > no change ... I'm not clear on this so I don't have an opinion right now > > I'm going to take osapi.base out of the spec for v0.9 > > > > - if (osapi.people.getViewer) { > should be > if (osapi.people && osapi.people.getViewer) { > since you have to test for both the service and the method. > > fixed > > > > - "When no parameters are specified, the server defaults will be used: > { user : @me, group : @self}. This is built on > osapi.newJsonRequest()." > Two notes. It's userId: @me, groupId: @self. And what it's built > on is an implementation detail that the spec should leave out. > > fixed > > > > - osapi.people.get.execute(callback) > isn't a great description, since there is no such function on > osapi.people.get. Need some central documentation describing the > execute() method on all request and batch objects > > no change - not sure what the change should be here > > will add central documentation about "Request objects" (which have > execute() methods) and "Batch Request objects" (which have execute() and > add() methods) > > > > - osapi.activities.create(params) - maps to the activities create > endpoint. This is a non-ui intermediated version of the > osapi.ui.requestCreateActivity call. > I think the distinction between UI-intermediated calls and > non-UI-intermediated calls is artificial. A container may make *any* > JS call ui-mediated - permission dialogs being the obvious example. > Strongly prefer if we: > (1) Kill requestCreateActivity(), and for that matter everything in > osapi.ui > (2) Leave UI mediation up to the container, in particular use > activities.create() instead of requestCreateActivity() > (3) If we have time, build the message service API for the > requestSendMessage()/requestShareApp(), if not leave that for v.Next > (4) Don't bother with requestPermission() in this API, though that > opinion is 99% based on experience with a few containers, and other > container developers may have very different feedback. > > no change, but I'm +1 on these comments > > goodbye osapi.ui > > > > - osapi.appdata.deleteData > Since JS prevents us from just calling this "delete", I'd rather > call it "remove()", which is at least a predictable naming pattern. > Or "deleteAppdata()". > > no change, but I'm +1 > > Bob thinks he can get delete() to work...if not, we'll use remove() > > > > - " osapi.messages TODO" > I think this means it's not in 0.9. > > removed > > > > * osapi.newBatch() - returns a new osapi.base.batch request. > * osapi.base.batch.add(key, request) - Adds a service request to > the batch associated with the specified key. These may be jsonrpc > calls, such as osapi.people.get(), or osapi.makeRequest calls to third > parties, or a mixture of the two. > * osapi.base.batch.execute(callback) - executes all of the > requests in the batch. Callback is passed a JSON object mapping each > request key to a JSON response object. > > Don't doc that batches are in "osapi.base.batch". All we need to doc > is that batch objects have add() and execute() methods. > > no change, I think this makes sense though, esp if we take out the > osapi.base docs > > see commnet above about documenting "Batch Request objects" > > > > - Batch functions:" > > One thing I would recommend adding is tweaking osapi.newBatch() to be: > osapi.newBatch(opt_json) > ... where opt_json is a JSON array describing a full request. This > lets developers that are truly fluent with JSON-RPC write: > osapi.newBatch([{method: "people.get"} ... etc...]).execute(callback); > ... and done. > > no change, seems reasonable > > Let's punt on this for now...I don't think many developers will use it > > > > > - osapi.makeRequest > This should be consistent with everything else in osapi, not with > gadgets.io. I think the right API is to follow Data Pipelining: > > osapi.http.get(params) > osapi.http.post(params) > > (and any other HTTP methods we care to specify at this point). > "params" should exactly match the attributes on os:HttpRequest, and > must use the same data format returned by os:HttpRequest (which I > haven't specified yet, I will send out that today). It also shouldn't > mention that it uses gadgets.io.makeRequest(). > > no change, but I like it > > I'm planning to create methods for head, get, put, post, delete > > > > - osapi.ui: > > As said above, I think this needs to be killed, as ui-mediated is up > to the container, not the spec. Batching semantics get a little > tricky, but we're far better off supporting batching for ui-mediated > requests than not, because it gives the container a lot more > flexibility for good UIs - instead of showing 5 dialogs in succession, > the container can show a single dialog saying the gadget's trying to > do 5 things. For now, we can leave the batch semantics up to the > container, as I can come up with a variety of plausible semantics. > > Also, everything else is method(params).execute(callback), and there's > no reason to break down and use positional parameters here. > > no change - the ui namespace does seem strange since there's nothing that > says the container *has* to make these calls user-mediated > > I think we can get rid of the ui namespace. I agree that we should > operate under the assumption that any js call can be ui-mediated. > > > > - osapi.base: > > I don't think any of these need to be documented. If containers > offer additional service methods, it's up to containers to support > getting those methods into osapi. The SystemHandler method you > describe above is reasonable. > > no change. I don't fully understand this, but if containers don't need > to be consistent on the osapi.base namespace, then we can leave this out > > > > - Error Handling: > > I like the pattern of using and propagating "error" through the > structures. A big +1, as it's simple and clear. That said, the doc > needs to be explicit that: > - "error" may not be used as a key to batch.add(). > - "error" is reserved and may not be used as a property of a JSON > RPC batch response item. > > fixed > > > Also, when the doc says "The error handling works exactly as it does > for the JSONRPC interface, since this api is based on that protocol." > that's not true. In JSON-RPC, a network error means that you get a > JSON structure with just an error: > {error: {message: "", code: ""}} > ... without any per-request-item content. What you're doing - which I > like - is creating dummy response items at each key and copying the > network error into each response item. > > no change - not sure how to update > > > > In "Example: Processing any sub requests in the batch that succeeded: > ", the example is incorrect. We don't have an error property within > each returned activity, since all activities are in a single request. > We need an example like: > > var batch = osapi.newBatch(). > add('v', osapi.people.getViewer()). > add('vf', osapi.people.getViewerFriends()); > > batch.execute(function(result) { > for (var name in result) { > if (result.hasOwnProperty(name)) { > if (result[name].error) { > gadgets.log("Found an error: ", result[name].error.message); > } else { > gadgets.log("Got some data: ", result[name]); > } > } > } > }); > > fixed > > > > > > On Mon, Mar 30, 2009 at 12:15 PM, Bob Evans <[email protected]> wrote: > > > > The URL: http://wiki.opensocial.org/index.php?title=OSAPI_Specification > > > > On Mon, Mar 30, 2009 at 12:01 PM, Bob Evans <[email protected]> wrote: > >> Hi, > >> > >> I have updated the OS Lite spec doc to match the current > >> implementation (there is a follow on patch coming today with the > >> metadata-based discovery of service methods). > >> > >> Known issues: > >> > >> * requestCreateActivity and activities.create. It might be possible to > >> make all calls ui-interruptible, but this prevents batching, well, > >> it makes batching a lot stranger and more involved. For example, what > >> happens if I have a batch where 2 of 4 calls require UI interaction > >> with the user. It's do-able, but would require a lot more thought than > >> .9 allows, I think. My proposed solution keep requestX methods, and > >> discuss making all service calls ui-intermediable in the next version > >> spec discussion. > >> > >> * send vs execute vs call naming. I prefer execute, but if people want > >> a shorter name, then send seems good. Some have suggested call as a > >> name, but this is used in the language, so seems like a bad idea. > >> > >> * Rename osapi.makeRequest to osapi.http.get/post. To me, makeRequest > >> reflects the existing, underlying gadgets calls, but I could see > >> making it http. > >> > >> * osapi.ui namespace is weird, better suggestions? > >> > >> * Should we have a bucket of params for the requestX calls, or > >> positional params as it is now? Maybe all of this is obviated if we > >> make the requestX calls be jsonrpc calls. > >> > >> > >> Thanks, > >> Bob > >> > > > > > > > > > > > > > > > > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ > You received this message because you are subscribed to the Google Groups > "OpenSocial and Gadgets Specification Discussion" group. > To post to this group, send email to > [email protected] > To unsubscribe from this group, send email to > [email protected]<opensocial-and-gadgets-spec%[email protected]> > For more options, visit this group at > http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en > -~----------~----~----~----~------~----~------~--~--- > >
