Brock Pytlik wrote:
http://cr.opensolaris.org/~bpytlik/ips-2670-v2/
Here's the next webrev. It's synced with Shawn's putback, and covers
most of the comments that J and Bart had. One thing that hasn't gone in
yet are the comments that were requested. I plan to do those tomorrow.
It also moves all the files touched to have copyright 2009 on them.
--------------------
src/client.py:
--------------------
line 913: I really think return_actions should be False by default.
I believe most users are going to expect a list of packages as the
result instead of a list of actions -- it's what I plan on doing for the
web interface. Any objections?
lines 966-968: This will never raise an exception?
lines 979-1001: it seems like bits of this code could be split into
nested functions that you could call and share among search display.
Right now, it's hard to follow. Some comments would also be useful.
line 1028: An api_errors.CertificateError could happen here for a
remote search on an HTTPS server, right?
--------------------
man/pkg.1.txt:
--------------------
line 222: s/authorities/publishers/
line 230: s/case sensitive/case-sensitive/
lines 235-250: two spaces after the end of each sentence please
line 238: paragraph break before "Boolean search"
line 244: paragraph break before "By default"
line 245: s/it's/its/
lines 245-246: "AND an OR also work on packages." -- confusing
wording. Is this supposed to be "AND and OR" ?
line 251: Some search examples would be nice in the EXAMPLES section
towards the end.
--------------------
src/modules/actions/directory.py
--------------------
line 58: s/"Empty path attribute"/_("Empty path attribute")/
--------------------
src/modules/actions/file.py
--------------------
line 68: s/"Empty path attribute"/_("Empty path attribute")/
--------------------
src/modules/actions/generic.py
--------------------
lines 271-284: two spaces after the end of each sentence please
--------------------
src/modules/actions/link.py
--------------------
line 57: s/"Empty path attribute"/_("Empty path attribute")/
--------------------
modules/client/api.py
--------------------
line 986: s/self.img.get_uuid(pub.prefix)/pub.client_uuid/
line 990: s/pub["origin"]/origin/
--------------------
modules/client/api_errors.py
--------------------
line 384: actually, by convention, there should be two blank lines
between each class
line 401: needs _()
line 402, 405, 410, 414, 418, 419, 421: s/auth/pub/
line 454: are you missing a _ in front of the () ?
--------------------
modules/client/query_parser.py
--------------------
lines 22-23: I think the header is not quite right. See lines 23-27
of modules/client/progress.py for the correct one.
lines 80, 82, 99, 104, 106, 108, 113: s/'/"/g
line 235: drop this line?
line 250: drop spaces around '='
lines 261-262: move this up to 258 and then drop 269-270
--------------------
modules/indexer.py
--------------------
lines 67-69, 113-114: are these really meant to be public properties?
lines 78-87, 90-95, 164, 168, 378, 530: s/'/"/g
lines 164-170: are there any concerns about the size of this file and
memory footprint since the whole file is being read into memory?
line 248: s/file_handle.tell()/str(file_handle.tell())/
lines 255, 262, 279: s/"%s" % cur_location/cur_location/
lines 292: if you drop the "found" variable here, you could change
the loop to use the "for: else:" construct and avoid the assignment entirely
lines 544-545: This should raise an ApiException instead of a
RuntimeError.
--------------------
modules/query_parser.py
--------------------
lines 22-23: I think the header is not quite right. See lines 23-27
of modules/client/progress.py for the correct one.
--------------------
modules/search_errors.py
--------------------
lines 22-23: I think the header is not quite right. See lines 23-27
of modules/client/progress.py for the correct one.
line 75: insert newline here
--------------------
modules/search_storage.py
--------------------
lines 22-23: I think the header is not quite right. See lines 23-27
of modules/client/progress.py for the correct one.
lines 149-150, 249: should raise an ApiException; not RuntimeError
--------------------
modules/server/catalog.py
--------------------
line 297: extra newline; drop
--------------------
modules/server/depot.py
--------------------
lines 251-252: please format the same lines 318-319
line 263: please don't assert; raise a BadRequest error instead and
use cherrypy.log() to log something meaningful
lines 287, 314: s/'/"/g
line 298: str() instead of "%s" ?
--------------------
modules/server/query_parser.py
--------------------
lines 22-23: I think the header is not quite right. See lines 23-27
of modules/client/progress.py for the correct one.
--------------------
modules/server/repository.py
--------------------
lines 332-337: With this change, this function no longer yields
search results as they are generated. Was this change intentional?
Have you checked modules/server/api.py search() and the web-based search
in the templates provided by the depot to be certain that everything is
still works correctly? I would think this would be a bump in the
version of pkg.server.api at the least.
--------------------
src/pkgdefs/SUNWipkg/prototype
--------------------
Are you still going to split SUNWply into its own package? It needs to be.
--------------------
src/tests/cli/t_api_search.py
--------------------
line 52: dead code?
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
