Merge lp:~jelmer/launchpad/can-upload-webcall into lp:launchpad
Proposed by
Jelmer Vernooij
Status: | Merged |
---|---|
Approved by: | Curtis Hovey |
Approved revision: | no longer in the source branch. |
Merged at revision: | 10949 |
Proposed branch: | lp:~jelmer/launchpad/can-upload-webcall |
Merge into: | lp:launchpad |
Diff against target: |
137 lines (+72/-1) 4 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+4/-0) lib/lp/soyuz/interfaces/archive.py (+26/-0) lib/lp/soyuz/model/archive.py (+14/-0) lib/lp/soyuz/stories/webservice/xx-archive.txt (+28/-1) |
To merge this branch: | bzr merge lp:~jelmer/launchpad/can-upload-webcall |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Nelson (community) | Approve | ||
Curtis Hovey (community) | code and rc | Approve | |
Review via email: mp+26141@code.launchpad.net |
Commit message
Expose IArchive.
Description of the change
This exposes the IArchive.
To post a comment you must log in.
Hi Jelmer,
As discussed, my main questions were regarding the return value... we should never be raising 500's... there are a bunch of descriptive webservice_error exceptions in interfaces/ archive. py that you might be able to re-user (or add new ones).
I'd suggest returning True if they can upload (as returning nothing is kinda weird), and raising the a descriptive webservice error otherwise.
Regarding the wrapping of the method for the API, I can see why you've done so (rather than changing the signature of checkUpload to use strings and updating all the call-sites), but prefixing with an '_' seems strange to me... if there are other precedents for this fine, but otherwise, I'd expect something like checkUploadAPI() or similar to be (1) a bit more descriptive, and (2) not imply that it should be treated as protected? What do you think? Let me know if you find any other examples of methods being wrapped for API arguments.
-Michael
{{{ ubuntu/ member/ ncommander] has joined #launchpad-reviews /code.edge. launchpad. net/~jelmer/ launchpad/ can-upload- webcall/ +merge/ 26141 for reference :) ). error(xxx) as in the other examples in interfaces/ archive. py) oadRights exception defined there - it might be what you're after.
16:15 < noodles775> jelmer_: are there other examples of wrapping a method for api exposure that prefix with an _?
16:15 < noodles775> (in ref. to your checkUpload branch).
16:16 < sinzui> noodles775, I think that means you want to rename the method to be public now
16:16 < sinzui> noodles775, I will refrain from suggesting a uses the rename hack since exposing a method explicitly makes it public
16:17 -!- NCommander [~mcasadeva@
16:17 < jelmer_> noodles775: Maybe, I haven't actually checked...
16:17 < jelmer_> noodles775: I'll have a look now.
16:18 < noodles775> sinzui: in this case it seems it's just to use string args instead of the normal IFaces... it seems strange to prefix with _ imo. (sinzui: https:/
16:22 < noodles775> jelmer_: also, why are you not returning True/False? It also seems strange to not return *anything* to mean yes, and raise an exception otherwise?
16:24 < jelmer_> noodles775: Originally I had it return the exception string and None when uploading was allowed.
16:25 < jelmer_> noodles775: I guess it really depends on whether we would want to expose the reason why the upload would not be possible to the user
16:25 < jelmer_> noodles775: there are different exception classes so in theory you could have different behaviour based on that.
16:27 < noodles775> jelmer_: we should never be expecting a 500, afaik (if you do raise an exception, it needs to use webservice_
16:29 < jelmer_> noodles775: ok
16:29 < noodles775> jelmer_: in fact, there's already an InsufficientUpl
16:30 < noodles775> jelmer_: but I'm still unsure why you're not returning True/False.
16:30 * noodles775 checks for other examples.
16:30 < jelmer_> noodles775: With True/False the user doesn't get an idea why the upload wouldn't be allowed (user not in packageset, pocket is release and distroseries is closed, etc)
16:31 < jelmer_> noodles775: The feature request doesn't require that though, so I'm happy to change it to a boolean.
16:32 < noodles775...