On Mon, Apr 13, 2009 at 02:43:04PM -0700, Brock Pytlik wrote:
> [email protected] wrote:
>> 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.
This is the original comment:
This class exists so that the classes the parent class query
parser uses to build the AST are the ones defined in this module
and not the parent class's module. This is done so that a
single query parser can be shared between the client and server
modules but will construct an AST using the appropriate classes.
I think I'm confused by the relationship between this class's existence
and the behavior of the parent class's query parser. There's definitely
a missing word in the first sentence. Is it the following?
This class exists so that the classes __that__ the parent class
query parser uses to build the AST are the ones defined in this
module and not the parent class's module.
I could also see 'of' in place of 'that', but that makes the rest of the
sentence make no sense. If the above is correct, perhaps it would be
more clear to say this:
This class overrides a variety of methods that are defined in
the parent class. These subclassed methods are how the client's
query parser builds its AST.
Or have I missed the point completely?
>> - 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.
I think either of these would work well.
>> - 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.
Sounds good. Thanks for fixing that.
>> 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.
Perhaps it isn't necessary, but it would be clearer to refine this
further. I didn't completely understand what this meant until I read
the response from you. How about:
The action type and key indexes, which are necessary for
efficient searches by type or key, store their file handles in
dictionaries. File handles for actions are in at_fh, while
filehandles for keys are kept in st_fh.
>> - 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.
I really wasn't trying to be rhetorical here. I was trying to figure
out if you had imposed a specific type requirement on your lists -- by
subclassing maybe? However, it sounds like you were trying to explain
that the function expects a list with a pre-determined format of its
contents. If the latter is true, consider stating this in a way that
expresses that the lists must be in a format that the callee expects.
Perahps I'm being obtuse, or pedantic, but type implied class/instance
identity, which isn't what you mean.
>> - 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..."
Okay. It might be worthwhile to change this to "the string, named
input, into..." That way you'll avoid a miscue by your less astute
readers. (Me)
>> - 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.
In that case, perhaps it would be better as, "Text is the variable that
contains the toakens and syntax of the query."
>> - 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.
Since you've gone to the trouble of writing it here, there's no reason
why we couldn't modify the docstring. How about: (starting at 457)
The variable 'restriction' is a generator of actions, over which
the search shall be performed. Only boolean queries will set
restriction. Queries that are returning packages do not set
restriction. Nodes that return packages have, by definition,
parents that must return packages. This means that only queries
contained within a boolean query higher up in the AST tree will
have restriction set.
Or did I misunderstand how this works?
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
