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

{{{
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@ubuntu/member/ncommander] has joined #launchpad-reviews
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://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141  for reference :) ).
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_error(xxx) as in the other examples in interfaces/archive.py)
16:29 < jelmer_> noodles775: ok
16:29 < noodles775> jelmer_: in fact, there's already an InsufficientUploadRights exception defined there - it might be what you're after.
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> jelmer_: in which case, would either returning True, or raising a descriptive webservice_error be better?
16:34 < jelmer_> noodles775: I would expect a descriptive webservice error to be better because it gives the user more information that they can use or ignore.
16:34 < noodles775> jelmer_: great. So if they can upload, return True, otherwise raise a descriptive webservice error.
}}}