[Webkit-unassigned] [Bug 92155] A script to analyze class dependencies for webkit code and patches

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=92155





--- Comment #3 from Dirk Pranke <dpranke at chromium.org>  2012-08-13 09:58:27 PST ---
(From update of attachment 154131)
This set of feedback is almost all stylistic; I thought it would be easier to follow my comments if we separated that out from any substantive comments on the algorithms and features.



View in context: https://bugs.webkit.org/attachment.cgi?id=154131&action=review

> Tools/Scripts/webkit-classdep:6
> +#import networkx as nx

if you're not using this I'd just leave it out ...

> Tools/Scripts/webkit-classdep:8
> +import os

also, group all of the imports from the standard library and put them in alphabetical order

> Tools/Scripts/webkit-classdep:11
> +    print "Usage: wbkit-classdep <command> [options]";

python statements don't need semicolons. Also, typo "wbkit" :).

> Tools/Scripts/webkit-classdep:12
> +    print "";

You should strongly consider following the pattern we use in webkit-patch for a script with subcommands.

> Tools/Scripts/webkit-classdep:21
> +    print "See 'webkit-classdep help COMMAND for more information on a specific command";

top-level objects (functions, classes, etc) should be separated by two blank lines, not one.

> Tools/Scripts/webkit-classdep:53
> +class GraphVizOutput:

classes should derive from object unless there's a good reason not to

> Tools/Scripts/webkit-classdep:55
> +        pass;

you can omit the __init__ method if it doesn't do anything.

> Tools/Scripts/webkit-classdep:87
> +    absolutePath = os.path.abspath(path);

we have a set of abstractions we use for access to system services, e.g., webkitpy.common.host and webkitpy.common.system.filesystem you should probably. It will make it easier to write tests for things.

> Tools/Scripts/webkit-classdep:97
> +        while (nextLine != ""):

this can just be 

while nextLine:
   if ...

> Tools/Scripts/webkit-classdep:103
> +                    included = included.strip('"');

this is probably better handled with a regexp match (using the re package)

> Tools/Scripts/webkit-classdep:111
> +        gypFile = open(gyp, "rt");

you can use the 'with open(gyp, "rt") as gypfile': 

Also, you don't need the 'r', and I'm not even sure 't' is a legal mode option?

> Tools/Scripts/webkit-classdep:115
> +            nextLine = nextLine.strip(" \t\n");

strip() strips whitespace by default.

> Tools/Scripts/webkit-classdep:140
> +            pass;

it's usually a bad idea to have catchall like this. in this case, it's not clear to me why you would want to catch anything. If your concern is that the directory won't exist, it would be better to test for that explicitly with os.path.exists().

> Tools/Scripts/webkit-classdep:158
> +def canonicalName(fullPath, includeDirs):

i'd probably rename this or at least put in a comment; "canonical" can mean a lot of different things, and the way you're using it (which I think is to find the relative path from an include directory) isn't a common one.

> Tools/Scripts/webkit-classdep:228
> +    visited = set([]);

you can just do set(), you don't need to pass a list.

> Tools/Scripts/webkit-classdep:409
> +def runHelp(argv):

option parsing should be done using the optparse package (or, if you adopt the webkit-patch style of subcommands, its variant on it).

> Tools/Scripts/webkit-classdep:420
> +# Main

the main code for this script should either be indented into an 'if __name__ == '__main__' block or moved into a separate main() function (which is then called from an if __name__ block.

The reason for this is that we want to cover as much code by tests as possible, and that means you need to be able to import the file without it doing anything.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.