Richard Lowe <richlowe at richlowe.net> writes:
> Hey all,
>
> A webrev is available for prelim code review:
>
> http://cr.opensolaris.org/~richlowe/scm-review_90
>
Not all of this is strictly review, but I thought it worth including
completely.
general
CAS changes should be done under a separate bug?
(though this integration, probably)
usr/src/tools/README.tools
This was updated in parts, but not fully
usr/src/tools/findunref/exception_list
This should be done as a rename to the new file with the most
commonality when moved into TeamWare.
usr/src/tools/findunref/findunref.py
30: Don't do a wildcard import of stat *
107: Comment should be docstring
107: added, modified, ws, exclude are unused in here.
(some should be, I think, some should be removed)
180: ws is unused
function comments should be docstrings
I'm having a hard time following some of this, I think because of
the various unused arguments that pretend to be meaningful then aren't.
usr/src/tools/onbld/Checks/CStyle.py
37: A fair amount of this block (the subprocess stuff) is common
with JStyle, but duplicated. Could fix that.
74: Could lose this block, it's there for testing.
usr/src/tools/onbld/Checks/Cddl.py
54: pylint complains about the lack of space in 'lambda x,y:'
87: The else clause here is pointless, since we only ever exit that
loop by exhausting the input
112 (and similar): These would probably be more readable if not
literally formatted
usr/src/tools/onbld/Checks/Comments.py
73: ''.isspace() == False, this won't catch spurious blank lines
108/116: The use of check_db here could be clearer.
137: This redefines the builtin 'id', I recall Mike having said he
preferred this to alternative var names.
usr/src/tools/onbld/Checks/Copyright.py
59: 'copyright' is also a python builtin, but I'm happy to shadow
it.
usr/src/tools/onbld/Checks/DbLookups.py
46: I don't think we need this, the Exception.__init__ stuffs its
args into self.args (would quiet pylint about the fact we're not
calling Exception.__init__), I think.
122: I still really want BooBug and Monaco to share a common
interface, rather than papering over it in BugDB. I think it'd
be considerably more tidy
272: I have no memory of why we try to avoid webrticli
283: Same comment as 46
usr/src/tools/onbld/Checks/JStyle.py
38: Same as CStyle.py:37
73: same as Cstyle.py:74
usr/src/tools/onbld/Checks/Rti.py
67: [nit] gatePath.rstrip('/')
usr/src/tools/onbld/Checks/__init__.py
34: Is __all__ correct here?
51: Is there a better place to put onSWAN()
57: onSWAN() should catch a specific exception, or set thereof
usr/src/tools/onbld/Scm/Backup.py
350: I think John Levon had concerns about us including Mq patches in this
usr/src/tools/onbld/Scm/WorkSpace.py
26: Better as a module-level docstring?
54/55/58: As comments
Likely, comment 55 matters, 54 is somewhat stale, and 58 is deferable.
362: This does a bunch of work by hand it really shouldn't need to
We already know the revisions in the AL, we can find the ones
with a single parent that is not itself in the AL easily enough
without having to walk through history.
Ordering would need to be looked into carefully, however.
491: pylint doesn't like the lack of space in 'lambda x,y:'
515: As comment (matching comment on line 58)
651: Hardly code review, but I sure do wish this took the same args
as diff.
usr/src/tools/onbld/hgext/cdm.py
24: See bug #303
107: See WorkSpace.py:651
285: Do we care about this, as compared to cdm_list?
427: 'n' is not used.
469: Do we want to do this while TeamWare is still primary?
508: What this is described to do and what it does don't quite line up
This checks if your entire set of outgoing changes amounts to
nothing, not each individual cset.
Is this wrong in the case of file-less merges?
usr/src/tools/scripts/Makefile
73: jstyle is out of alphabetical order
usr/src/tools/scripts/cddlchk.py
89: Bad indentation space v. tab
usr/src/tools/scripts/copyrightchk.py
See bug #479
usr/src/tools/scripts/hg-active.py
See bugs #479, #502
73: Should this accept paths other than the workspace root?
usr/src/tools/scripts/rtichk.py
See bug #479
usr/src/tools/scripts/ws.sh
85: Should this have been removed with the 4.x bits?
usr/src/tools/scripts/wx2hg.sh
115: See bug #462
usr/src/tools/scripts/webrev.sh
See bug #209
usr/src/tools/scripts/webrev.1
See bug #208
usr/src/tools/scripts/nightly.sh
See bug #481
2109: This bug (186), may now be fixed in mercurial?
(old) 2199: Should this really go away entirely? What about the
TeamWare case?
-- Rich
