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. :)
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?
- 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).
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.
- line 968: typo: controlls should be controls
- line 969 and 975: Even though these are identifiers, I think they
should still be capitalized. It makes it very hard to find the
beginnings and ends of sentences without this.
- line 969 - 974: This is a run-on sentence. A possible revision:
Return_type is an enumeration that describes the type of the
information that will be converted. If return_type is action
information, tup is a three-tuple of the fmri name, the match, and a
string representation of the action. In the case where return_type
is package information, tup is a one-tuple containing the fmri name.
api.py:
- line 954: Capitalize this identifier, too.
api_errors.py:
- line 446: That should probably be RuntimeError instead of
RunetimeError. No apostrophe is needed here. RuntimeErrors is
fine.
- line 485: Capitalize pkg, please.
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.
- 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?
- lines 111-118: Please captialize the identifiers that are at the
beginnings of these sentences.
- line 131: hypenate this as client-only
- line 134: affected instead of effected
- 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.
- lines 153 and 154: This seems a bit redundant. How about writing
this as:
This function performs the local client-side search.
- lines 154-162: Please start sentences with capital letters.
- lines 154 and 155: insert a 'the' between over and results
- line 157: What does the it in this sentence refer to?
- 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.
- lines 194, 195, 252, 253, 254, 266, 267: Please capitalize these sentences.
indexer.py:
- line 56: Place a comma between disk and which
- line 113: Should be either "This dictionary" or "These dictionaries"
- line 114: make "action type" consistent with whichever choice you
make above.
- line 186: Add commas after 'tokens' and 'them'
- lines 187 and 188: Please capitalize these identifiers.
- line 347: Is it possible to have a list that's not of type list?
- line 463, 464, 588, 598, 600, 601, 603, 737, 788, 807, 808, 809:
Please capitalize these identifiers
- lines 807 and 808: Is this backwards? Generally the source contains
the old information and the destination is where the new information
goes.
query_parser.py:
- line 174: Capitalize xterms
- line 213: Capitalize zap as it is the first word in the new
sentence
- line 215: "the implict wildcard" or "an implicit wildcard"?
- line 276: Orterms returns the union ... ?
- line 300: Parse the string and input into ...
- line 316: Singlular / plural disagreement here. Writing
this as the following might help:
Text strings are the tokens and syntax of the query.
- line 316-323: Capitalize the indentifiers that are at the beginning
of these sentences, please.
- line 378: "Method that produces a sort key for an action," perhaps?
- line 431: typo: Distrubtes should be Distributes
- line 447: Might be more succinct as, "Returns true if query supports
version 'v'."
- line 457: Capitalize the identifier at the beginning of the sentence
- line 457-458: This is difficult to read, and contains two whiches.
Is it correct to say the following?
Restriction is a generator function that returns actions within the
search domain.
- line 458-460: I don't understand what this means. Can you explain
it a different way?
- line 486-490: Ditto for the last three comments above
- line 507: Did you mean serve instead of server?
- line 510: Comma after possible and general; this is a parenthetical
statement, right?
- line 512: Comma after below
- I'm giving up on trying to catch all of the instances where
capitalization is needed. I'd appreciate it if you could fix these
as you pass through again, though.
- line 622 and 623, 944 and 945: Too many whiches in this sentence too.
- line 643: Did you mean search instead of searched?
- line 862 and 870: Hypenate to in-memory
- line 979: Does it read these sequentially, or does it only read
sequential offsets? Please clarify.
- line 1003 - 1005: A possible revision:
If offsets is equal to None, match all possible results. A match
with no results is represented by an empty set.
server/api.py:
- line 139: typo: indexe
server/query_parser.py
- lines 35-40: same as comments for client/query_parser lines 41-43
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
