2008/6/2 Danek Duvall <[EMAIL PROTECTED]>: > On Sun, Jun 01, 2008 at 10:27:13PM -0500, Shawn Walker wrote: > >> > - line 72: you're not actually doing a version check here. Is there a >> >> I hadn't thought about checking the version, but I really should. The >> current code will only work with 3.0.3 but less than 3.1.0 (not yet >> released at last check). >> >> > reason that you're bothering to explicitly wrap the import? >> >> Yes, it was suggested by Krister. > > Okay (I skipped most of that subthread, since I was away). It's not clear > to me that this is useful -- on a system that hasn't been hacked to bits, > it'll be there (otherwise SUNWipkg won't even install). I'll want this to > go away, though, certainly once cherrypy gets properly integrated.
I have no objections to removing the check completely if so desired. Just let me know. >> > - on line 17, you return a status code, but on line 191, you raise an >> > exception. What's the difference? >> >> ilne 17? I'm not sure what line you're referring to. However, I tried >> to keep most of this code the same it was before. So if it is >> returning one now that's probably why. Can you clarify? > > Errr. Not sure about my original intended line number, but line 79 will > do. Basically, I can't tell why in some cases you're raising exceptions to > make the server return a particular HTTP status code and why in some cases > you're doing so via a return value. Is there a useful difference between > the two methods? If not, shouldn't they all be done the same way? This was in transaction.py. In the case of transaction.py, the open() function is returning a particular status to the calling function. The calling function is explicitly evaluating the return value and then performing exceptions or various other bits of logic based on that return value. As such, it wasn't appropriate for me to raise an exception there. As for abandon, it was originally sending a response directly to the client, so I migrated it to do the same in CherryPy nomenclature, since the calling code is expecting abandon to handle it. So, in short, it's just doing what it used to. My personal preference is to let the caller handle abandon() and return the status since that would make it match open()'s behaviour. Would that be your desired approach? >> > - Do you think it would be feasible / cleaner to decorate each of the >> > operations methods with attributes designating what operation and >> > version they represent, and then build the vop table out of those >> > decorations, rather than the method names? My lack of understanding of >> > what's going on in default() might preclude that. >> >> It sounds like it would be possible. However, a pointer to the actual >> method is still needed for CherryPy's dispatch table. >> >> You can find more information about how its default dispatcher works here: >> http://www.cherrypy.org/wiki/CherryPyTutorial#Findingthecorrectobject >> >> and here: >> http://www.cherrypy.org/wiki/CherryPyTutorial#Partialmatchesandthedefaultmethod >> >> Of course a custom dispatcher could be written which would certainly >> change things. For simplicity's sake, I was trying to stick with the >> default one provided. > > Okay, I'll take a look, thanks. I'm certainly not going to hold your wad > back for this sort of exploration. I would be more than willing to revisit the dispatcher issue as a later enhancement, as I'm not terribly thrilled with the dummy object either. >> > SUNWpython-cherrypy/Makefile: >> > >> > - I wonder if perhaps doing the tarball download higher up, in the main >> > "make install" wouldn't be better -- so that it'll work out of the box, >> > and you could integrate it into "make link" and so on. >> >> Eventually cherrypy will go away and not be part of the build, so I'm >> not certain. > > Yeah, but we don't have a timeframe on that yet, do we? Seems like we > could be in the current situation for a while. True. So what are your expectations then in regards to this wad? >> > - line 34, 35: This needs to point to us/Sun, since we're the distributor >> > (vendor) here. >> >> Ah. I changed the vendor, what about the email? I've removed it for now. > > Email probably should be whatever it is for SUNWipkg. No EMAIL is specified, so I'll leave it out. >> I have posted an updated webrev here: >> http://cr.opensolaris.org/~swalker/pkg-depot-9/ >> >> ...and a udiff of the changes from the last webrev here: >> http://cr.opensolaris.org/~swalker/pkg-depot-9/raw_files/pkg-depot-9.patch > > Thanks. I'll look over them now. Thanks for the re-review. I'll get to these changes later tonight (I hope). Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
