2008/5/30 Danek Duvall <[EMAIL PROTECTED]>: >> http://cr.opensolaris.org/~swalker/pkg-depot-8/ > > src/depot.py: > > - line 45: I think this should be "opensolaris.org". The auth name isn't > the FQDN of the server, and we should be more biased towards opensolaris > than towards sun.com.
That was the original value. However, I agree and have changed it. > - line 55, 59: are these things that we'd want to be able to tune? THREADS_MIN and THREADS_MAX are not expected to be tuned. They are only used as a way to ensure that someone doesn't start the depot server with a bad number of threads. CherryPy does not currently support dynamic adjustment of the number of threads, meaning it starts all the threads specified at server startup. However, I could understand the desire to change SOCKET_TIMEOUT_DEFAULT. I have added a new option (-t) and updated the documentation and smf manifest to reflect this. > - 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. > - line 113: why not use sets? Existence in the set is the equivalent of > the get returning True. Changed. > - line 137: I think I'd rather just see three except clauses, even if you > have to call usage() in three separate places. I admit, it would be cleaner. Changed. > - line 147: do this as a try/except block Changed. > - line 164: I think this is an exit(1). 2 should be for option > processing errors, as per the man page. I didn't even notice that on the man page. Thanks, changed. > - line 176: use double-quotes here, and no space before colons in dict > literals. Plus we've been using four-space indents here. Changed. > - line 198: space around equals. Changed. > catalog.py: > > - Why not just define outfile() in terms of output() -- just write > everything you get from output()? Same in updatelog.py. Changed. > server/__init__.py: > > - Might as well torch the SCCS ident line here. Changed. > server/depot.py: > > - It would be really useful to have a block comment explaining what this > file is for -- why it was grabbed from the cherrypy code, and what > changes you've made to it. I'm not sure if the comment at line 74 is > that comment, but if so, it should probably be moved up to the top and > made more obvious that it's not part of the original code. The comment at line 74 is basically the reason, however, I have added a clarifying docstring to the new class. > server/face.py: > > - line 154, 159: don't need ".keys()" here -- the "in" operator works on > the dictionary itself as you'd expect. Changed. > - line 163: doesn't this call need three arguments? Oops. Fixed. > server/transaction.py: > > - line 176, 177: spaces around equals Fixed. > - 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? > updatelog.py: > > - line 359: as long as you're doing other cleanup here, might as well > change "it" to "is" here. Fixed. > server/repository.py: > > - line 78: no need for parens around return values Changed. > - line 89: continuation indents are four spaces Fixed. > - line 98: maybe a short comment on why we need an object, rather than an > empty string, or None, or some other core Python object? Added. > - line 141: I'm confused on how we might get here. It seems like no > matter what you do, unless you get shunted into the face code above, > we're going to error out of this method. I think you're right; it looks like a leftover from the old logic. > - line 294: Why not just catch socket.error directly? True, that was rather silly of me. Fixed. > - I assume there's no decorator to tell cherrypy not to decode URL > components? Not that I know of. If you want the raw request path, you can get it from request.path_info or from the environ. > - 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. > 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. > - If nothing else, your "prototype" target rule should be broken up into > more rules. I'm not sure what you wanted, but I have split it up and simplified it some more. > - line 41: you're missing an "IPS" here. > > SUNWpython-cherrypy/copyright: Changed. > - Why is the CDDL here? I was copying the approach from the other files. I have removed it and the Sun copyright notice. Please correct me if I'm wrong. > SUNWpython-cherrypy/pkginfo: > > - line 25: no trailing "thon" in the package name. Fixed. > - line 30: This needs to always be "1.0". Fixed. > - 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. > Danek > Thanks for reviewing this! I re-ran pylint on the files I changed and did some additional, minor cleanups. 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 Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
