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 <sse...@myspace-inc.com> 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:* opensocial-and-gadgets-s...@googlegroups.com [mailto: > opensocial-and-gadgets-s...@googlegroups.com] *On Behalf Of *Lane > LiaBraaten > *Sent:* Thursday, April 02, 2009 9:45 PM > *To:* opensocial-and-gadgets-s...@googlegroups.com > *Cc:* shindig-dev@incubator.apache.org > *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 <lliab...@google.com> > 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 <awi...@gmail.com> 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 <bobev...@google.com> wrote: > > > > The URL: http://wiki.opensocial.org/index.php?title=OSAPI_Specification > > > > On Mon, Mar 30, 2009 at 12:01 PM, Bob Evans <bobev...@google.com> 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 > opensocial-and-gadgets-s...@googlegroups.com > To unsubscribe from this group, send email to > opensocial-and-gadgets-spec+unsubscr...@googlegroups.com<opensocial-and-gadgets-spec%2bunsubscr...@googlegroups.com> > For more options, visit this group at > http://groups.google.com/group/opensocial-and-gadgets-spec?hl=en > -~----------~----~----~----~------~----~------~--~--- > >
