Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1329983002:
Make a json file expresses diff (Closed)
DescriptionMake a dictionary that expresses a diff between two json files including AST.
BUG=530402
Patch Set 1 #
Total comments: 29
Patch Set 2 : new #
Total comments: 30
Patch Set 3 : Upload only make_diff.py on this patch. #Patch Set 4 : Upload only make_diff.py on this patch. #
Total comments: 20
Patch Set 5 : Updated diffing script based on previous code review #
Total comments: 22
Patch Set 6 : Updated diffing script based on shiino-san's advice #Patch Set 7 : Need to decide this script's name #
Total comments: 29
Patch Set 8 : Uploaded diffing script based on haraken-san's advice #
Total comments: 3
Patch Set 9 : several nit modifications were made #Patch Set 10 : Added 'Name' into interface object #Patch Set 11 : Changed the name 'OTHER_THAN_MEMBER_TYPES' to 'DIFF_INSENSITIVE_FIELDS' #Messages
Total messages: 23 (8 generated)
shimadaa@google.com changed reviewers: + bashi@chromium.org, haraken@chromium.org, yukishiino@chromium.org
https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... File Source/bindings/scripts/make_file_of_idl_diff.py (right): https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:1: import json nit: Add shebang #!/usr/bin/env python https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:5: nit: remove two blank lines. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:5: Add a comment which describes what this script does like Natuko did in https://codereview.chromium.org/1323493002/diff/160001/Source/bindings/script... https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:8: """Load a json file into a dictionary nit: Put period at the end of the summary line. nit: Add one empty line between the summary line and "Args" line. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:10: filepath : A file path of a json file that we want to load nit: Remove space before colon. Same for below. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:13: with open(filepath,'r') as f: nit: Add a space after comma. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:16: def remove_same_interface(new_loaded_file_data, old_loaded_file_data): |new_loaded_file_data| and |old_loaded_file_data| are a bit long. How about |new_data| and |old_data|? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:25: for interface_name, interface_content in new_loaded_file_data.items(): nit: |interface_content| -> |interface| nit: new_loaded_file_data.items() -> new_loaded_file_data.iteritems() https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:27: if interface_content == old_loaded_file_data[interface_name]: This comparison would be expensive. Ideally we want to avoid this kind of comparison. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:30: return [new_loaded_file_data, old_loaded_file_data] nit: use tuple. (new_loaded_file_data, old_loaded_file_data) https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:32: def remove_name_and_filepath(file_data): remove_name_and_filepath -> remove_unused_fields? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:33: """Remove "Name" and "FilePath" data Please describe why you need to remove these fields. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:37: for interface_content in file_data.values(): nit: |interface_content| -> |interface| https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:39: if member_name == 'Name' or member_name == 'FilePath': nit: if member_name in ['Name, 'FilePath']: ... https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:45: def add_annotation(comparison_source_file, comparison_object_source_file, diff_tag): |comparison_source_file| -> |target| |comparison_object_source_file| -> |opponent|? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:45: def add_annotation(comparison_source_file, comparison_object_source_file, diff_tag): |add_annotation| -> |annotate|? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:48: comparison_source_file : A dictionary that we want to add diff_tag It would be better to have a definition of "A dictionary". https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:54: extattributes_and_members = ['ExtAttributes', 'Const', 'Attribute', 'Operation'] This should be a constant. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:58: for extattributes_or_member in extattributes_and_members: nit: I guess that we may not need to add annotation for members if an interface doesn't exist in the another dict. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:79: removed_file_data = remove_same_interface(new_loaded_file_data, old_loaded_file_data) nit: You can re-write this like below if you use a tuple in remove_same_interface(). new_loaded_file_data, old_loaded_file_data = remove_same_interface(...) https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:86: return [added_diff, deleted_diff] nit: use tuple https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:89: def merge_diff(added_diff, deleted_diff): I may be wrong, but could you explain why diff = deleted_diff.copy() diff.update(added_diff) return diff doesn't work? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:98: extattribute_and_members = ['ExtAttributes', 'Const', 'Attribute', 'Operation'] This is the same as line 54. This should be shared. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:104: for extattribute_or_member in extattribute_and_members: |extattribute_or_member| -> |member_type|? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:105: for extattributes_or_member_content in interface_content[extattribute_or_member]: |extattributes_or_member_content| is long. I would name it |member| even it could have extended attributes. https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:111: def make_json_file(diff_data, filepath): |make_json_file| -> |write_diff|? https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:119: f.close Remove f.close https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:127: added_and_deleted_diff = make_added_and_deleted_diff(new_loaded_file_data, old_loaded_file_data) added, deleted = make_added_and_deleted_diff(...) https://codereview.chromium.org/1329983002/diff/1/Source/bindings/scripts/mak... Source/bindings/scripts/make_file_of_idl_diff.py:131: make_json_file(merged_diff, 'merged.json') What is the expected output here? Don't we need interfaces which both |new_loaded_file_data| and |old_loaded_file_data| have?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Uploaded two scripts (make_diff.py, print_diff.py) based on advice from bashi-san. Please give me reviews when you have time. Thank you in advance.
I didn't take a look at print_diff.py. Let's try to reduce 'nit' comments in next round(s) :) https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... File Source/bindings/scripts/ayumi.py (right): https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/ayumi.py:1: import json Please remove this file from the CL. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:3: """Usage: make_diff.py new_file.json old_file.json diff_file.json It's difficult to understand what new_file.json and old_file.json contain. Could you add explanations? https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:7: import sys, os import os import sys https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:12: from utilities import merge_dict_recursively I don't think we need above three line. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:17: { Interface: { ExtAttributes: [{'Name': '...'}, Interface -> 'InterfaceName' ExtAttributes -> 'ExtAttributes' https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:45: {...}, It's not clear what {...} means here. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:46: { ExtAttributes: [{...},...,{...}], Don't we need 'InterfaceName' at the beginning? ExtAttributes -> 'ExtAttributes' https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:52: { ExtAttributes: [{...},...,{...}], I think we can remove the last block (and the previous one, maybe). https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:67: Args: A file path of a json file that we want to load nit: Args: filepath: ... https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:68: Return: A "interfaces" that is loaded from the json file An "interfaces" object? https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:68: Return: A "interfaces" that is loaded from the json file nit: break line after colon. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:74: def remove_unused_fields(file_data): file_data -> interface https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:76: Arg: A "interfaces" consist of json file data nit: Arg -> Args: Args: interface: An interface object https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:77: Return: A "interfaces" removed "Name" and "FilePath" data from the input nit: break line after colon. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:96: annotated_target = target.copy() annotated_target -> annotated https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:96: annotated_target = target.copy() I don't think you need to copy the original dict. annotated = {} for interface_name, interface in target.iteritems(): if interface_name in opponent.keys(): annotated[interface_name] = {} for member_type in EXTATTRIBUTES_AND_MEMBER_TYPES: for member in interface[member_type]: if not member in opponent[interface_name][member_type]: member['diff_tag'] = diff_tag annotated[interface_name][member_type] = member else: ... https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:116: """Make two dictionary(One is added annotations 'added', the other is added anntaitons 'deleted'). How about: Make 'added' and 'deleted' diffs from given two interfaces. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:117: Args : nit: Remove space before colon. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:118: new_loaded_file_data: A "interfaces" consists of new json file data loaded by load_json_file() new_interfaces: An interfaces object https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:119: old_loaded_file_data: A "interfaces" consists of old json file data loaded by load_json_file() old_interfaces: An interface object https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:120: Return : nit: Remove space before colon. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:131: def merge_diff2(added_diff, deleted_diff): Remove this if you don't need this. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:153: In this function, import merge_dict_recursively from utilities.py. Is this still true? https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:161: for interface_name, interface_content in added_diff.items(): interface_content -> interface https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:170: nit: Add one empty line. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:171: def write_diff(diff_data, filepath): diff_data -> diff https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:172: """Make a json file consists of diff dictionary. How about: Write |diff| as json to |filepath|. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:190: nit: Add one empty line. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... File Source/bindings/scripts/make_file_of_idl_diff.py (right): https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_file_of_idl_diff.py:1: import json Please remove this file from the CL.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:15: { 'InterfaceName': { 'ExtAttributes': [{'Name': '...'}, nit: Indents are broken. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:17: {'...'}], nit: Remove {'...'} https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:22: {'...'}], nit: Remove {'...'} https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:29: {...}], nit: Remove {'...'} https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:41: {...}] nit: Remove {'...'} https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:63: def remove_unused_fields(interfaces): btw, do we really need this? https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:70: removed_file_data = interfaces.copy() Stop copying or use deepcopy() because you modify |interface| later. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:77: def annotate_to_members(old_interface, new_interface): annotate_to_members() -> members_diff() ? https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:99: #members.remove(member) nit: remove https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:122: def annotate(old_interfaces, new_interfaces): annotate() -> interfaces_diff() ?
https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:3: """Usage: make_diff.py new_file.json old_file.json diff_file.json Let's make the script take old_file.json first. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:5: old_file.json: A json file including idl data of old Chrome version No description about diff_file.json. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:14: In this script, we call a dictionary like below 'interfaces'. Do not indent with 3 spaces. """Data structure of ... In this script, ... Also, we call ... // 80 columns ... // next 80 columns """ https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:18: { 'InterfaceName': {'ExtAttributes': [{'Name': '...'}, No need of a space between "{" and "'InterfaceName':". https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:49: No need of an empty line. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:53: EXTATTRIBUTES_AND_MEMBER_TYPES = ['ExtAttributes', 'Consts', 'Attributes', 'Operations'] Let's define something like: DIFF_TAG = 'diff_tag' DIFF_TAG_ADDED = 'added' DIFF_TAG_DELETED = 'deleted' https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:60: Return: s/Return/Returns/ https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:73: Return: Returns: (annotated, is_changed) where annotated: ... is_changed: ... https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:75: is_annotated: A return variable "annotated" is annotated or not What about the followings? is_changed: True if two interfaces have a different member. is_changed: False if two interfaces are identical, otherwise True. is_changed: True if two interfaces are NOT identical. Or, you can reverse the definition of the flag. is_identical: True if two interfaces are identical, otherwise False. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:83: if member in old_interface[member_type]: We have to check the difference in more details. It seems your code doesn't detect changes of operation's arguments. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:109: return interface nit: The return value is not used + the comment says nothing about the return value. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:141: Args : nit: indent and space
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
I uploaded diffing script based on shiino-san's advice. Please give review if I need to modify it. Thank you in advance. https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/60001/Source/bindings/scripts... Source/bindings/scripts/make_diff.py:67: Args: A file path of a json file that we want to load On 2015/09/10 03:35:26, bashi1 wrote: > nit: > > Args: > filepath: ... Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:15: { 'InterfaceName': { 'ExtAttributes': [{'Name': '...'}, On 2015/09/11 00:23:45, bashi1 wrote: > nit: Indents are broken. Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:17: {'...'}], On 2015/09/11 00:23:46, bashi1 wrote: > nit: Remove {'...'} Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:22: {'...'}], On 2015/09/11 00:23:46, bashi1 wrote: > nit: Remove {'...'} Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:29: {...}], On 2015/09/11 00:23:45, bashi1 wrote: > nit: Remove {'...'} Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:41: {...}] On 2015/09/11 00:23:46, bashi1 wrote: > nit: Remove {'...'} Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:63: def remove_unused_fields(interfaces): On 2015/09/11 00:23:45, bashi1 wrote: > btw, do we really need this? Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:70: removed_file_data = interfaces.copy() On 2015/09/11 00:23:45, bashi1 wrote: > Stop copying or use deepcopy() because you modify |interface| later. Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:77: def annotate_to_members(old_interface, new_interface): On 2015/09/11 00:23:45, bashi1 wrote: > annotate_to_members() -> members_diff() ? Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:99: #members.remove(member) On 2015/09/11 00:23:45, bashi1 wrote: > nit: remove Done. https://codereview.chromium.org/1329983002/diff/120001/Source/bindings/script... Source/bindings/scripts/make_diff.py:122: def annotate(old_interfaces, new_interfaces): On 2015/09/11 00:23:45, bashi1 wrote: > annotate() -> interfaces_diff() ? Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:3: """Usage: make_diff.py new_file.json old_file.json diff_file.json On 2015/09/17 03:26:57, Yuki wrote: > Let's make the script take old_file.json first. Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:5: old_file.json: A json file including idl data of old Chrome version On 2015/09/17 03:26:56, Yuki wrote: > No description about diff_file.json. Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:14: In this script, we call a dictionary like below 'interfaces'. On 2015/09/17 03:26:56, Yuki wrote: > Do not indent with 3 spaces. > > """Data structure of ... > In this script, ... Also, we call ... // 80 columns > ... // next 80 columns > """ > Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:18: { 'InterfaceName': {'ExtAttributes': [{'Name': '...'}, On 2015/09/17 03:26:57, Yuki wrote: > No need of a space between "{" and "'InterfaceName':". Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:49: On 2015/09/17 03:26:56, Yuki wrote: > No need of an empty line. Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:53: EXTATTRIBUTES_AND_MEMBER_TYPES = ['ExtAttributes', 'Consts', 'Attributes', 'Operations'] On 2015/09/17 03:26:56, Yuki wrote: > Let's define something like: > DIFF_TAG = 'diff_tag' > DIFF_TAG_ADDED = 'added' > DIFF_TAG_DELETED = 'deleted' Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:73: Return: On 2015/09/17 03:26:56, Yuki wrote: > Returns: > (annotated, is_changed) where > annotated: ... > is_changed: ... Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:75: is_annotated: A return variable "annotated" is annotated or not On 2015/09/17 03:26:57, Yuki wrote: > What about the followings? > > is_changed: True if two interfaces have a different member. > > is_changed: False if two interfaces are identical, otherwise True. > > is_changed: True if two interfaces are NOT identical. > > Or, you can reverse the definition of the flag. > > is_identical: True if two interfaces are identical, otherwise False. Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:109: return interface On 2015/09/17 03:26:57, Yuki wrote: > nit: The return value is not used + the comment says nothing about the return > value. Done. https://codereview.chromium.org/1329983002/diff/140001/Source/bindings/script... Source/bindings/scripts/make_diff.py:141: Args : On 2015/09/17 03:26:57, Yuki wrote: > nit: indent and space Done.
LGTM from my point of view. Please get a LGTM from bashi, too. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:18: In this script, we call a dictionary like below 'interfaces'. Also, we call one single quotes (') => double quotes (") https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:83: is_changed: True if two interfaces are identical, otherwise False True if two interfaces are **NOT** identical, otherwise False
LGTM with a couple of comments. Good work! https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:6: """Usage: make_diff.py old_file.json new_file.json diff_file.json make_diff.py sounds a bit too general and it's not clear what it does. Shall we rename it to generalte_idl_diff.py or something? https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:7: old_file.json: A json file including idl data of old Chrome version Before explaining the parameters, let's explain what the script does. - make_diff.py is a script that generates a diff of two given IDL files. Also let's explain that old_file.json and new_file.json are input and diff_file.json is output. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:20: element in "members" "member". Slightly better: The format of the json files is as follows. Each json file contains multiple "interface"s. Each "interface" contains 'ExtAttributes', 'Consts', 'Attributes' and 'Operations'. Each item in them are called a "member". https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:66: filepath: A file path of a json file that we want to load filepath: A json file path would be enough. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:68: An "interfaces" object that is loaded from the json file An "interfaces" object loaded from the json file https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:75: """Create a diff between two "interface" objects by adding annotations into Nit: into => to https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:108: def annotate_to_all_members(interface, diff_tag): annotate_to_all_members => annotate_all_members https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:109: """Add annotations into all "member" objects of |interface|. Nit: into => to https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:125: adding annotations (DIFF_TAG_ADDED or DIFF_TAG_DELETED) into each member Nit: into => to https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:131: An "interfaces" object expressing diff between |old_interfaces| and expressing diff => representing a diff https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:153: """Make a json file that consists of diff dictionary. Write a diff dictionary to a json file. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:155: diff: An "interfaces" object that expresses a diff expresses => represents https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:163: old_json_file = argv[0] Can we add: if len(argv) != 3: print "Usage: make_diff.py <old_file.json> <new_file.json> <diff_file.json>" exit() ?
LGTM with nits. Good job! 1. No tests? I prefer having a test if we are going to land this before https://codereview.chromium.org/1323493002/ is landed. 2. Please update the description like: Add generate_idl_diff.py This script generates a diff of IDL files from two input json files. See comments in the script for details. https://codereview.chromium.org/1329983002/diff/240001/Source/bindings/script... File Source/bindings/scripts/generate_idl_diff.py (right): https://codereview.chromium.org/1329983002/diff/240001/Source/bindings/script... Source/bindings/scripts/generate_idl_diff.py:6: """Usage: make_diff.py old_file.json new_file.json diff_file.json Let's put description first then usage. For example, """generate_idl_diff.py is a script ... Usage: generate_idl_diff.py ... old_file.json: ... new_file.json: ... """ https://codereview.chromium.org/1329983002/diff/240001/Source/bindings/script... Source/bindings/scripts/generate_idl_diff.py:168: '<diff_file.jsson>' + '\n') '<diff_file.json>\n' https://codereview.chromium.org/1329983002/diff/240001/Source/bindings/script... Source/bindings/scripts/generate_idl_diff.py:169: exit() exit(1)
https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... File Source/bindings/scripts/make_diff.py (right): https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:7: old_file.json: A json file including idl data of old Chrome version On 2015/09/17 13:16:38, haraken wrote: > > Before explaining the parameters, let's explain what the script does. > > - make_diff.py is a script that generates a diff of two given IDL files. > > Also let's explain that old_file.json and new_file.json are input and > diff_file.json is output. Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:18: In this script, we call a dictionary like below 'interfaces'. Also, we call one On 2015/09/17 10:57:39, Yuki wrote: > single quotes (') => double quotes (") Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:20: element in "members" "member". On 2015/09/17 13:16:38, haraken wrote: > > Slightly better: > > The format of the json files is as follows. Each json file contains multiple > "interface"s. Each "interface" contains 'ExtAttributes', 'Consts', 'Attributes' > and 'Operations'. Each item in them are called a "member". Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:66: filepath: A file path of a json file that we want to load On 2015/09/17 13:16:38, haraken wrote: > > filepath: A json file path > > would be enough. Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:68: An "interfaces" object that is loaded from the json file On 2015/09/17 13:16:38, haraken wrote: > > An "interfaces" object loaded from the json file Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:75: """Create a diff between two "interface" objects by adding annotations into On 2015/09/17 13:16:38, haraken wrote: > > Nit: into => to Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:83: is_changed: True if two interfaces are identical, otherwise False On 2015/09/17 10:57:39, Yuki wrote: > True if two interfaces are **NOT** identical, otherwise False Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:108: def annotate_to_all_members(interface, diff_tag): On 2015/09/17 13:16:38, haraken wrote: > > annotate_to_all_members => annotate_all_members Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:109: """Add annotations into all "member" objects of |interface|. On 2015/09/17 13:16:38, haraken wrote: > > Nit: into => to Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:125: adding annotations (DIFF_TAG_ADDED or DIFF_TAG_DELETED) into each member On 2015/09/17 13:16:38, haraken wrote: > > Nit: into => to Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:131: An "interfaces" object expressing diff between |old_interfaces| and On 2015/09/17 13:16:38, haraken wrote: > > expressing diff => representing a diff > Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:153: """Make a json file that consists of diff dictionary. On 2015/09/17 13:16:38, haraken wrote: > > Write a diff dictionary to a json file. Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:155: diff: An "interfaces" object that expresses a diff On 2015/09/17 13:16:38, haraken wrote: > > expresses => represents Done. https://codereview.chromium.org/1329983002/diff/220001/Source/bindings/script... Source/bindings/scripts/make_diff.py:163: old_json_file = argv[0] On 2015/09/17 13:16:38, haraken wrote: > > Can we add: > > if len(argv) != 3: > print "Usage: make_diff.py <old_file.json> <new_file.json> <diff_file.json>" > exit() > > ? > Done.
still LGTM
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329983002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329983002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
bashi-san: Would you help ayumi-san re-upload a CL with the merged repository?
On 2015/09/25 03:40:49, haraken wrote: > bashi-san: Would you help ayumi-san re-upload a CL with the merged repository? Sure. doing. |
