Re: [pkg-discuss] Code review for 7364 please (all comments and formatting)

Fri, 10 Apr 2009 15:08:14 -0700 

Brock Pytlik wrote:

Webrev:
http://cr.opensolaris.org/~bpytlik/ips-7364-v1/


src/client.py:
  lines 934, 949, 968, 969: two spaces after '.' between sentences please


  line 935: I can't help but think that variables referenced in the 
comments should have ' or " around them to make it clear they aren't 
part of the sentence's grammar.  This applies to all of your changes 
(not just this file or place).


  lines 936, 968: s/controlls/controls/


  line 992: i know this isn't a code review, but "prefix" in pub seems 
likely to break once the dictionary compatibility layer that's in place 
now for publisher objects is removed, maybe hasattr or just try to get 
it's pub.prefix value and catch the AttributeError?


src/modules/actions/attribute.py:
  line 63: two spaces after '.' between sentences please

src/modules/actions/depend.py:
  line 242: two spaces after '.' between sentences please

src/modules/actions/directory.py:
  line 168: two spaces after '.' between sentences please

src/modules/actions/driver.py:
  line 795: two spaces after '.' between sentences please

src/modules/actions/file.py:
  line 381: two spaces after '.' between sentences please

src/modules/actions/group.py:
  line 112: two spaces after '.' between sentences please

src/modules/actions/legacy.py:
  line 152: two spaces after '.' between sentences please

src/modules/actions/legacy.py:
  line 152: two spaces after '.' between sentences please

src/modules/actions/license.py:
  line 149: two spaces after '.' between sentences please

src/modules/actions/link.py:
  line 110: two spaces after '.' between sentences please

src/modules/actions/user.py:
  line 198: two spaces after '.' between sentences please

src/modules/client/api.py:
  line 945, 986, 987, 1137: two spaces after '.' between sentences please

  line 1137: s/what's //


  lines 1137-1138: s/This helps to catch situations where search is 
talking to a server which isn't a search server at all./This ensures 
that search is really communicating with a search-enabled server./


src/modules/client/api_errors.py:
  line 446: s/RunetimeError's/RuntimeErrors/

  lines 477, 485: two spaces after '.' between sentences please


src/modules/client/query_parser.py:

  line 46: insert a newline here after docstrings to separate them from 
code or comments -- this applies everywhere, so I won't denote the other 
cases (and other files)


  line 134: s/effected/affected/ -- I think

src/modules/indexer.py:

  lines 212, 217, 221, 224, 227, 464, 588, 598, 600, 601, 603, 687, 
688, 690, 691, 703, 705, 737, 756, 788, 807, 808, 809, 810: two spaces 
after '.' between sentences please


  line 213: s/axes/axis/ ?

  line 216: insert newline here for readability

  line 231: s/which indicated which/that indicate which/

  line 396: s/depo/repo/

  line 598: remove leading whitespace for docstrings


src/modules/manifest.py:
  lines 370, 371: two spaces after '.' between sentences please

src/modules/query_parser.py:

  lines 50, 150, 215, 307, 316, 438, 488, 494, 508, 509, 511, 514, 610, 
619, 620, 622, 642, 721, 745, 756, 825, 873, 1004: two spaces after '.' 
between sentences please


  lines 290-296: s/ : /: / (no spaces before : in dicts)

  line 323: replace 'querier' with 'queryer' or 'caller' perhaps?

  line 431: s/Distrubtes/Distributes/

  line 519: s/OR's/ORs/

  line 610: s/us/is/

  line 644: s/token)/token) /

  line 662: s/fullying/fully/


  lines 1092-1136: can this be broken up into smaller pieces?  7 or 8 
indent levels is a bit hard to follow :|


src/modules/search_storage.py:
  lines 210-211: something went awry with the formatting here


  lines 211, 212, 213, 215, 216, 243, 245, 297, 298, 301, 302, 337, 
338, 341, 342, 370, 575, 576, 577, 579, 580: two spaces after '.' 
between sentences please


src/modules/server/api.py:
  line 139: s/indexe/index/

  line 146: two spaces after '.' between sentences please

src/modules/server/depot.py:
  lines 296, 297: two spaces after '.' between sentences please

src/modules/server/query_parser.py:
  lines 79, 80, 81, 83: two spaces after '.' between sentences please

src/modules/server/repository.py:
  line 329: two spaces after '.' between sentences please

Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss






















					Reply via email to