Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155523002:
Add mapping of directories to owners as json file (Closed)
|
Created:
4 years, 5 months ago by raikiri Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd mapping of directories to owners as json file
BUG=625258
Committed: https://crrev.com/3f9105b87e9563be67ea6d0318478dab634b4398
Cr-Commit-Position: refs/heads/master@{#406637}
Patch Set 1 #Patch Set 2 : add csv_to_json_function #Patch Set 3 : add function with basic functionality #Patch Set 4 : redo mapping json file #
Total comments: 11
Patch Set 5 : redo mapping #
Total comments: 4
Patch Set 6 : add customization options to csv_to_json #Patch Set 7 : correction #
Total comments: 5
Patch Set 8 : change json to only contain files in imported #
Total comments: 3
Patch Set 9 : Corrections #
Total comments: 1
Messages
Total messages: 31 (11 generated)
raikiri@google.com changed reviewers: + dcampb@google.com, qyearsley@chromium.org
On 2016/07/14 at 23:57:21, raikiri wrote: > lgtm, but before submitting, clean up the cl description. bug word should not be there if no bug is associated.
Description was changed from ========== Add mapping of directories to owners as json file BUG= ========== to ========== Add mapping of directories to owners as json file BUG=625258 ==========
Description was changed from ========== Add mapping of directories to owners as json file BUG=625258 ========== to ========== Add mapping of directories to owners as json file BUG= ==========
Description was changed from ========== Add mapping of directories to owners as json file BUG= ========== to ========== Add mapping of directories to owners as json file BUG=625258 ==========
On 2016/07/15 at 00:26:20, dcampb wrote: > On 2016/07/14 at 23:57:21, raikiri wrote: > > > > lgtm, but before submitting, clean up the cl description. bug word should not be there if no bug is associated. Yep, that's right; bug is http://crbug.com/625258; I've now added that in there.
A few questions/comments: 1. How was this generated? If you used a script to convert from some format to this JSON, then might that script be useful in the future? If so, we could commit it here as well. 2. We're probably only interested in tests in "imported/" for the auto-import process. 3. "Guesses" doesn't need to be a sub-dict, and probably doesn't need to be included. Number of tests is probably also not necessary. 4. The fields in the JSON file don't have to be the same as in the sheet; for example, "Email alias for notifications about flaky tests" is a little bit verbose; similarly, "Component (if clear)" could be shorter. It might handy to have some sort of script to automatically convert from the source sheet (perhaps downloaded as CSV) to the subset of that info that we want in this JSON file, to make it easy for others to update info in that sheet, and then update our JSON mapping. What do you think?
Looks good -- Did we want to do some transformations, e.g.: "Owning team" -> "team" "Email alias for notifications about flaky tests": "notification-email", "Guesses/contact" -> "other-contact" "Component (if clear)" -> "component" "Directory" -> "directory" "Number of tests" -> ignore? Also, when using this, we may find it convenient if the JSON file is a map from directory to dict (with owners, component and preferences), but having a list of dicts like this, one for each directory, is OK. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/csv_to_json (right): https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:5: """Converts csv files to machine usable json One blank line can be added above the docstring. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:7: TODO(raikiri): DO NOT SUBMIT without a detailed description of csv_to_json. This message can be removed. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:13: import argparse Imports should be sorted. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:15: def main(argv): Formatting note: usually there's two blank lines before/between top-level functions or classes; so another blank line could be added above. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:18: help='the path to the csv') the path to the input CSV file https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:20: help='the output file name') The help lines might look good if capitalized and have punctuation, e.g.: parser.add_argument('filename', metavar='filename', help='The path to the input CSV file.') parser.add_argument('-o', '--output', help='The output file name.') Since this script isn't going to be used often, you can also optionally remove the "-o" abbreviation, and just pass the output as --output whenever the script is invoked. https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:21: args = parser.parse_args(argv) The argparse library can also access sys.argv by itself, so if you don't pass argv to parser.parse_args it also works. The main advantage of explicitly taking in args and passing it on to parse_args is that you can pass example args in unit tests. But for this script, it's not so helpful to have unit tests (since it's relatively simple and functionality can be tested by invoking the script directly). https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:28: jsonfile = open(out, 'w') jsonfile -> json_file https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:29: with open(filename) as csvfile: csvfile -> csv_file https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:30: readerobj = csv.DictReader(csvfile) readerobj -> reader https://codereview.chromium.org/2155523002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:34: We can have two blank lines between the last function and the `if __name__ == '__main__'` section.
https://codereview.chromium.org/2155523002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/csv_to_json (right): https://codereview.chromium.org/2155523002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:7: """ A few more little comments: - I personally prefer capitalizing initialisms like CSV and JSON. - "machine usable" can be written as "machine-usable" (or "machine-readable"). - Docstrings should generally be sentences with punctuation. - For one line docstrings, the closing quote can go on the same line. https://codereview.chromium.org/2155523002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:18: help='the path to the input CSV file.') Capitalize "The path to..." https://codereview.chromium.org/2155523002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:21: convert_csv_to_json(argv[0]) I think here you might want to do something like: args = argparse.parse_args() convert_csv_to_json(args.filename, args.output) Right? https://codereview.chromium.org/2155523002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/csv_to_json:31: dict_list.append(row) If we want to apply some transformation on each row (changing field names or filtering out field names), then it would go here, I think.
The CQ bit was checked by raikiri@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcampb@google.com Link to the patchset: https://codereview.chromium.org/2155523002/#ps120001 (title: "correction")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
One optional final feature that we may want to add: skipping test directories that don't start with imported/. This would make this JSON file much smaller, and we only really care about test directories in imported. There are trade-offs between making the script general and making the script convenient to use for this particular case of transforming this particular sheet. Making things general is good, although in this case I don't think we expect to use this for anything other than getting the directory_owner.json file and updating it later, so I think it's OK to make the script less general and more specific to this one use case. https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/csv_to_json (right): https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:6: """Converts CSV files to machine-readable JSON.""" I think it would be helpful give an example invocation here, showing how you invoked this script to generate the JSON. For example: Example usage: ./csv_to_json --filename=$HOME/input.csv --output=directory_owners.json \ --skip-keys="Number of tests" -f directory -f number-tests -f ... https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:20: parser.add_argument('-f', '--fieldnames', nargs='*', fieldnames -> field-names? https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:21: help='The ordered fieldnames of the CSV file. Defaults to first row.') fieldnames -> field names https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:33: reader = csv.DictReader(csv_file, fieldnames=field_names) fieldnames -> field_names https://codereview.chromium.org/2155523002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:43: main(sys.argv[1:]) four-space indent
LGTM with a couple suggested final changes https://codereview.chromium.org/2155523002/diff/140001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/csv_to_json (right): https://codereview.chromium.org/2155523002/diff/140001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:1: #!/usr/bin/python Possibly consider a more specific name for the script, e.g.: - generate_w3c_directory_owner_map - generate_imported_test_owner_map - generate_w3c_directory_owner_json - generate_imported_test_owner_json - convert_imported_test_owner_map All of these are a bit long, but I'd lead towards being longer and more specific, just for the benefit of people who come across this script later who don't have context about what it's for. https://codereview.chromium.org/2155523002/diff/140001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:11: --skip-keys="Number of tests" Could add a backslash at the end of this one (so that it works to copy and paste this into the terminal) Also, it's not necessary to indent example usage etc. https://codereview.chromium.org/2155523002/diff/140001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/csv_to_json:12: -f directory number-tests Also add "... team notification-email other-contact component", right?
The CQ bit was checked by raikiri@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcampb@google.com, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2155523002/#ps160001 (title: "Corrections")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add mapping of directories to owners as json file BUG=625258 ========== to ========== Add mapping of directories to owners as json file BUG=625258 Committed: https://crrev.com/3f9105b87e9563be67ea6d0318478dab634b4398 Cr-Commit-Position: refs/heads/master@{#406637} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3f9105b87e9563be67ea6d0318478dab634b4398 Cr-Commit-Position: refs/heads/master@{#406637}
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2155523002/diff/160001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/generate_w3c_directory_owner_json (right): https://codereview.chromium.org/2155523002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/generate_w3c_directory_owner_json:6: """Converts CSV files to machine-readable JSON. This docstring doesn't seem like the best one-line description of what this is for :). Also, this should probably document where these CSV files come from. And, given that most of the lines in the JSON file are empty, does it make sense to omit them and modify the code that reads the JSON to be able to deal with missing fields? I'd guess that that would at the very least, make the JSON file more readable. And we do want that file to be human-readable, right? Do we expect it to also be updated, or is the source of truth the database or spreadsheet that is producing the CSV file?
Message was sent while issue was closed.
On 2016/08/09 at 04:06:21, dpranke wrote: > https://codereview.chromium.org/2155523002/diff/160001/third_party/WebKit/Too... > File third_party/WebKit/Tools/Scripts/generate_w3c_directory_owner_json (right): > > https://codereview.chromium.org/2155523002/diff/160001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/generate_w3c_directory_owner_json:6: """Converts CSV files to machine-readable JSON. > This docstring doesn't seem like the best one-line description of what this is for :). > > Also, this should probably document where these CSV files come from. > > And, given that most of the lines in the JSON file are empty, does it make sense to omit them and modify the code that reads the JSON to be able to deal with missing fields? I'd guess that that would at the very least, make the JSON file more readable. And we do want that file to be human-readable, right? > > Do we expect it to also be updated, or is the source of truth the database or spreadsheet that is producing the CSV file? The CSV comes from a Google Sheet ("Team ownership of test directories"). I was thinking that that sheet is likely to continue to be updated in the future, and lots of people update that sheet, so for now it's most convenient for that to be the source of truth. But, if just the info related to w3c tests is here, and it includes some information that's not from that sheet (e.g. directory owners' preferences with regard to notifications and importing new failing tests), then it might make sense to make the JSON file the source of truth, in which case we should omit directories with missing values, and perhaps we shouldn't keep the script for converting from CSV to JSON in that case. I'll follow up with this in http://crbug.com/636596 :-) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
