Shawn Walker wrote:
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.
Comments I agreed with and fixed have been removed.
--------------------
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?
I think the web interface should have a button to click, but I have no
objections to it being set to packages by default.
I'd like to stick to -a being the default for another build or two so
that I can make sure that -p is doing what it really should be. I think
what's there right now won't match the users' expectations with compound
queries. I only realized this yesterday, and the fix to get the right
behavior isn't simple.
lines 966-968: This will never raise an exception?
It can raise exceptions from query construction, but that's all. I've
added in handlers for that case.
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.
Sure. There won't be any sharing but I'm fine to pull them out into a
separate function.
line 1028: An api_errors.CertificateError could happen here for a
remote search on an HTTPS server, right?
Doesn't appear that's the case. Should cert validity be being checked
here? How do I check cert validity for a single server? For one the user
has passed in on the command line?
--------------------
man/pkg.1.txt:
--------------------
line 251: Some search examples would be nice in the EXAMPLES
section towards the end.
Yep, but those may not make this wad.
--------------------
modules/client/query_parser.py
--------------------
lines 261-262: move this up to 258 and then drop 269-270
Can't, in one place we use pkg:/, in the other pkg:// because in one
location we're including an authority and the other we're not.
--------------------
modules/indexer.py
--------------------
lines 67-69, 113-114: are these really meant to be public properties?
For now, yep
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?
Nope. Right now it's fixed at 128M. I plan in a subsequent wad to make
this adjustable via an environment variable or command line argument.
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
I think I'll stick with the found construction, I think it's clearer.
lines 544-545: This should raise an ApiException instead of a
RuntimeError.
It can't raise an ApiException, you're in code common to the server and
client here. Also, the only way to hit this exception is for a caller of
this function, who lives inside the api, to pass in an unknown value. I
think this should remain a reuntime error. I can change it to an assert
if you prefer.
--------------------
modules/search_storage.py
--------------------
lines 149-150, 249: should raise an ApiException; not RuntimeError
Can't raise an apiException here, for same reason as above. I will
change these to throw other exceptions and catch and rethrow in the api
code.
--------------------
modules/server/depot.py
--------------------
line 263: please don't assert; raise a BadRequest error instead and
use cherrypy.log() to log something meaningful
Why log something?
--------------------
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?
That's not true. This returns a list of generators.
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.
I'll check. All I need to do is start a repo, point a browser at it and
see what happens when I do a search?
--------------------
src/pkgdefs/SUNWipkg/prototype
--------------------
Are you still going to split SUNWply into its own package? It needs
to be.
Yep, sorry, bed was calling last night before I got around to that.
Figured it wasn't something that would cause the GUI team integration
problems.
--------------------
src/tests/cli/t_api_search.py
--------------------
line 52: dead code?
yep
Cheers,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
