Summary: | Add script to merge run_benchmark jsons | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, commit-queue, dean_johnson, dewei_zhu, msaboff, saam, slewis, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Keith Miller
2019-04-19 10:52:14 PDT
Created attachment 367806 [details]
Patch
Created attachment 367807 [details]
Patch
Comment on attachment 367807 [details]
Patch
r=me
Comment on attachment 367807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367807&action=review Code looks good; small nits on Python coding style. > Tools/Scripts/merge-result-jsons:31 > + Nit: PEP8 asks that there is 2 lines in-between imports and function definitions (and 1 for methods). > Tools/Scripts/merge-result-jsons:34 > + return json.loads(contents.read()) Nit: You can just use `return json.load(contents)`. > Tools/Scripts/merge-result-jsons:40 > + if type(value1) is list: Nit: Python recommends using `isinstance` rather than comparing types directly: `if isinstance(value1, list):` > Tools/Scripts/merge-result-jsons:44 > + for key in list(value1.keys()) + list(value2.keys()): value1.keys() and value2.keys() already return lists. This can be written a bit more concisely as: `for key in (value1.keys() + value2.keys()):` > Tools/Scripts/merge-result-jsons:62 > + help="File to put the merged json into prints to standard out if nothing is passed") Nit: You're missing positional arguments for the list of json files. I guess you can just use the remaining args instead, but it'd be more clear to use argparse. https://docs.python.org/2/library/argparse.html This should be able to be implemented pretty easily. I think it would look something like: parser.add_argument('json_files', metavar='N', default=[], nargs='+', help='json files to merge') > Tools/Scripts/merge-result-jsons:72 > + f.write(json.dumps(result)) This can be written more concisely: with open(opts.o, 'w') as f: json.dump(result, f) Comment on attachment 367807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367807&action=review > Tools/ChangeLog:11 > + This patch also adds +x bits to the compare-results script so it Nice. I thought I did this but have also been annoyed by +x not being there. I must've landed through commit queue or something. > Tools/Scripts/compare-results:236 > + revert Created attachment 367824 [details]
Patch for landing
Comment on attachment 367824 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=367824&action=review > Tools/Scripts/merge-result-jsons:49 > + Nit: Missing newlines between each function definition. > Tools/Scripts/merge-result-jsons:72 > + result = mergeJSONs(list(map(readJSONFile, args.jsons))) Nit: `map` already returns a list. No need for an extra conversion Comment on attachment 367824 [details] Patch for landing Clearing flags on attachment: 367824 Committed r244469: <https://trac.webkit.org/changeset/244469> All reviewed patches have been landed. Closing bug. |