[email protected] wrote:
Brock,
On Fri, Apr 10, 2009 at 01:09:28PM -0700, Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-7364-v1/
Whoa, you added lots of comments. :)
Yep :)
General comments:
- In many of the actions/ routines you have a comment saying, "see
generic.py for more details." I didn't see any changes to generic.py.
Are you referring to pre-existing comments in that module?
Yeah. The information there covers it all (and was changed as part of
the original search v1 was), so I thought duplicating the comment in
each module when essentially it would be identical text seemed a bit silly.
- In much of your writing you use which. There's a certain amount of
debate, and pedantry, in writing about when it's appropriate to use
which and when it's appropriate to use that. I'm not going to force
you to use one interpretation over another, but the NYTimes takes a
rather rule-oriented approach
(http://topics.blogs.nytimes.com/2008/11/24/that-which-or-what/),
whereas the American Heritage Book of English Usage is a bit more
relaxed (http://www.bartleby.com/64/C001/062.html).
Thanks for the pointers :)
client.py:
- lines 952-957: This seems unclear, but perahps I just don't
understand. Where does match come from? It's a little weird that
we're comparing action instance in the client (line 961), I think.
Basically, there are 5 different bits of information that can be
returned from the union of all actions. The matching package is common
to all. All but attribute actions return (in order) the attribute type
matched (basename, path, etc...), the action type (file, directory,
link, etc...), the value for that key attribute ('/usr/bin/pkg' for
example), and the package name. Attribute actions are harder to deal
with. All matches match with the "name" attribute of a set action. So,
if the template above was followed, all matches for set actions would
look like: "name set description <pkg name>", which didn't seem
particularly useful. Instead, "description" is moved to where name is,
which makes sense to me in terms of the semantics, and a another bit of
information is, what I call the full value, is placed where the word
"description" was. The full value is roughly the text in the description
that the token searched on matched. For the reason that that was how I
thought to do it at the time, I collapsed the key string for other
actions and the full value for attribute actions into the same field in
the dictionary. That field is what the "match" variable is.
client/query_parser.py:
- lines 41-43: This sentence is missing some words that would allow
it to make sense. Did you mean the following?
This class overrides the base QueryParser class. When building the
AST, the client QueryParser needs to apply a different approach than
its parent class.
I'm not sure what words are missing. It applies exactly the same
approach (algorithm), but uses different building blocks for the AST.
- line 77: I was going to complain that the correct plural form was
indices, but according to Merriam Webster, either is correct.
:)
- lines 87 and 88: If this is where you handle the search for
specific query terms, where do you handle the search for general
query terms?
Is 'particular query term' better? 'base query term'? A query is made up
of several query terms, which themselves can be made of query terms. I'm
trying to express that this is the lowest level of query terms. These
terms are not made up of other terms.
- lines 131-134: You might consider re-writing this to offer a more
detailed explanation of the algorithm that's being used here. As I
understand it, the clients are actually taking a reference to the
current dictionary. New dictionaries are created by threads
performing updates. When the update thread finishes creating its
dictionaries, it places the new references into the
global_data_dict. Once that occurs, new readers get the updated
data. The stale data is garbage-collected after threads finish
reading it, and release their reference.
That's true, but I don't think this is the place for that info, that's
all happening in the modules/query_parser.py module.
- line 157: What does the it in this sentence refer to?
I've changed the second it to search.
- line 182-184: Consider re-writing this to be a little clearer. A
possible revision might be:
The generator res contains actions from packages that have been
removed from the image since the last index was built. This
function uses the actions from res to and remove any corresponding
data from the search results.
Except that's not right. res contains the results from search. The
information from data_fast_remove is used to strip items from res.
Proposes rewrite:
This function removes any results from the generator res
(the search results) that are actions from packages known to
have been removed from the image since the last time the index
was built.
indexer.py:
- line 114: make "action type" consistent with whichever choice you
make above.
This I don't understand. I made it "These dictionaries", I don't believe
further modification to "action type" is necessary.
- line 347: Is it possible to have a list that's not of type list?
Would you prefer arguments in place of list? I was trying to say, it
takes lists, and more than that, it takes lists with a specific type.
- lines 807 and 808: Is this backwards? Generally the source contains
the old information and the destination is where the new information
goes.
No, that's right. Source is the directory that contains the information
that's just been indexed/updated. Dest is the directory that information
is going into.
- line 300: Parse the string and input into ...
No, input is the variable name. So I do mean "Parse the string, input,
into an AST..."
- line 316: Singlular / plural disagreement here. Writing
this as the following might help:
Text strings are the tokens and syntax of the query.
Text is the variable name. I don't believe there is a disagreement here.
- line 458-460: I don't understand what this means. Can you explain
it a different way?
I can remove it, or I can have a fairly long explanation which would
roughly be this:
Restriction is only set by boolean queries, and currently only by AND
queries. Further, if this node is returning packages, then the query
containing it would also contain packages, and therefore not make use of
restriction (hence it would be none). Therefore, the only way
restriction is not None is when this query is contained within an AND
query higher up in the AST tree.
server/query_parser.py
- lines 35-40: same as comments for client/query_parser lines 41-43
I'll fix this when we converge on the fix for the other one.
-j
Thanks for taking a look :)
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
