Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2188733003:
Implement functionality to cc relevant owners. (Closed)
|
Created:
4 years, 4 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. |
DescriptionImplement functionality to cc relevant owners.
BUG=632798
Committed: https://crrev.com/cbef1bb50c86c0b7da771e346d2ed38318289867
Cr-Commit-Position: refs/heads/master@{#409102}
Patch Set 1 #Patch Set 2 : implement functionality to cc relevant owners #
Total comments: 10
Patch Set 3 : corrections #
Total comments: 4
Patch Set 4 : corrections #
Total comments: 6
Patch Set 5 : corrections #Patch Set 6 : corrections #
Total comments: 13
Patch Set 7 : corrections #Patch Set 8 : skcmsd #Patch Set 9 : corrections #
Total comments: 4
Patch Set 10 : corrections #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== something something ========== to ========== Implement functionality to cc relevant owners. ==========
raikiri@google.com changed reviewers: + dcampb@google.com, qyearsley@chromium.org
FYI, Variable names are just suggestions. https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:33: self.email_list = [] Why not assign these two to variables and return them in the script? Is there a benefit from making attributes?? https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:73: self.print_('## Attempting to CC relevant parties.') Just a comment change: 'Gathering directory owners email to CC'.. or something similar. https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:316: folders_changed.add(line) So I'm guessing this function: 1. Looks for the last '/' in the directory name and takes from 'imported..../' as the directory 2. Searches for that string in the directory_dict and if the directory hasn't already been added, it adds it to the email list. What happens if the directory has no notifications email set to it. Will it try and CC an empty string? folders_changed --> directories line --> directory_path https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:320: for dict_set in data: dict_set --> directory_dictionary
Is there a bug filed for this yet? https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:33: self.email_list = [] On 2016/07/27 at 20:54:57, dcampb wrote: > Why not assign these two to variables and return them in the script? > Is there a benefit from making attributes?? I agree, if these attributes were removed and the functions that set them (parse_imported_test_changes and parse_directory_owners) were instead changed to return the desired value, then that might overall be easier to understand. In general, the fewer attributes a class has, the easier it is to understand. Every attribute represents some state which could potentially be modified by any other method on the class; so, whenever you reason about what the value of an attribute might be, you might have to consider that maybe it might be modified somewhere else. Whereas, if you have functions that just take an input and have an output and have no side effects, then it may be easier to understand. https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:71: with open('directory_owners.json') as data_file: Would this work if the current working directory is not .../webkitpy/w3c/? We probably want to use self.finder (a WebKitFinder instance) to get the path to the file: data_file_path = self.finder.path_from_webkit_base( 'Tools', 'Scripts', 'webkitpy', 'w3c', 'directory_owners.json') https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:101: self.run(['git', 'cl', 'land', '-f']) Somewhat separate from this CL: If possible, it would be nice to extract the body of this if-clause (everything under `if self.auto_update`) to a separate method. It might also be nice to extract other sub-functions. We should consider doing this refactoring in a separate CL. https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:311: line = line.strip('third_party/WebKit/LayoutTests/') I wonder if there's a way that we can avoid having a literal path here. Some tools we can use: self.fs.relpath(path, start): returns the relative path to |path| from start self.finder.layout_tests_dir(): Returns the absolute path (I think) to the layout test dir self.finder.chromium_base(): Returns the absolute path (I think) to the chromium base dir So maybe something like: layout_tests_relative_path = self.fs.relpath(self.finder.layout_tests_dir(), self.finder.chromium_base()) test_path = self.fs.relpath(line, layout_tests_relative_path) But there may be a simpler way. https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:313: line = line[:directory_sep] This looks like it may be the same as: line = self.fs.dirname(line) You may want to change this to use a new variable name: test_directory = self.fs.dirname(line) https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): https://codereview.chromium.org/2188733003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:77: self.assertEqual(updater.email_list, ['--cc=me@gmail.com', '--cc=you@gmail.com']) Could you also add a test method for parse_directory_owners?
A few more little optional suggestions: https://codereview.chromium.org/2188733003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:71: directory_dict = self.parse_directory_owners(data_file) To make `parse_directory_owners` more single-purpose (and probably easier to test), maybe it could take the list of dicts parsed from the file, rather than the file object -- so this here would be: with open(data_file_path) as data_file: directory_dict = self.parse_directory_owners(json.load(data_file)) https://codereview.chromium.org/2188733003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:307: def parse_imported_test_changes(self, results, directory_dict): Some things which may make this function easier to read: - A docstring would be helpful - The argument `results` isn't very clear - The word "parse" in the method name makes it sound like you're return some structured data based on some string. Which is somewhat true, but I'd say the main thing that this function does is get email addresses to CC based on owners of files. Would it make sense to change this to something like `get_emails_to_cc(changed_files, directory_to_email_map)` where changed_files is a list of changed files? https://codereview.chromium.org/2188733003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:315: print test_path Remember to remove debugging print lines :-) https://codereview.chromium.org/2188733003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:318: email_list.append('--cc=' + directory_dict[test_path]) The name email_list sounds like it's just a list of emails, but it's actually a list of extra args to `git cl upload` to add email addresses to CC. It may be nice to change this to just return a list of email addresses, since it's easy to add "--cc=" to each of them later; for example: self.check_run(['git', 'cl', 'upload', ...] + ['--cc=' + email for email in email_list])
raikiri@google.com changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:72: self.print_('Gathering directory owners email to CC') To be consistent with other lines that are printed by deps_updater, we could prepend this with "##" and add a period. I actually don't know why "## " is prepended to the printed lines, besides to make them stand out more, I guess, but we may as well be consistent. https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:310: Turns the output from git cl try-results into a usable format. Nit: The body of the docstring should be lined up with the start of the """, rather than with the First sentence. https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:316: email address of the point of contact of it Nit: missing period. Also, you can probably remove "of it". https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:328: test_path = test_path.strip('../') Side note: Be careful with strip, it probably doesn't behave quite as you expect it to. The argument to strip is a "character set" and as much as possible is stripped off. For example: >>> '../../.././foo/x..'.strip('./') 'foo/x' https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:346: return directory_dict BTW, if you wanted to keep this in the class (for organizational purposes), you can make it a static method (with the @staticmethod decorator), and then it doesn't get a self argument. This is no difference in behavior, just a difference in organization. https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:77: self.assertEqual(updater.generate_email_list(results, owners), ['me@gmail.com', 'you@gmail.com']) Test will need to be updated, I think.
On 2016/07/28 at 23:46:33, qyearsley wrote: > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:72: self.print_('Gathering directory owners email to CC') > To be consistent with other lines that are printed by deps_updater, we could prepend this with "##" and add a period. > > I actually don't know why "## " is prepended to the printed lines, besides to make them stand out more, I guess, but we may as well be consistent. > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:310: Turns the output from git cl try-results into a usable format. > Nit: The body of the docstring should be lined up with the start of the """, rather than with the First sentence. > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:316: email address of the point of contact of it > Nit: missing period. Also, you can probably remove "of it". > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:328: test_path = test_path.strip('../') > Side note: Be careful with strip, it probably doesn't behave quite as you expect it to. The argument to strip is a "character set" and as much as possible is stripped off. For example: > > >>> '../../.././foo/x..'.strip('./') > 'foo/x' I see but I don't believe this should present a problem with the output of git diff master --name-only > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:346: return directory_dict > BTW, if you wanted to keep this in the class (for organizational purposes), you can make it a static method (with the @staticmethod decorator), and then it doesn't get a self argument. > > This is no difference in behavior, just a difference in organization. > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): > > https://codereview.chromium.org/2188733003/diff/60001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:77: self.assertEqual(updater.generate_email_list(results, owners), ['me@gmail.com', 'you@gmail.com']) > Test will need to be updated, I think.
Could you file a bug for this task? Also, remember that you can respond to individual reviewer comments, and if you respond with "done" or some other comment, this can make it easier to track which comments you've responded to so far :-) https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:98: self.print_('No Failures, landing patch.') Note: I believe committing means the same thing as landing in this context, so this could be kept as "committing" if you want. https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:310: Turns the output from git cl try-results into a usable format. This takes the output from `git diff master --name-only`, right? https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:314: point in the import process. Nit: This wording may make it sound like it's a list of strings rather than a string with newline-separated file paths -- this might be clearer if you said something like "A string with newline-separated file paths relative to the repository root". https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:315: directory_dict: A mapping of directories in imported to the To make this clearer: "in imported" -> "in the LayoutTests/imported directory" https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:316: email address of the point of contact of it. "of it" can be removed. https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:320: import. Nit: extra period. https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:75: results = 'foo/bar/file.html\nfoo/bar/otherfile.html\nfoo/baz/files.html' Nit: This test makes it appear as though the file paths in `results` are starting from the same directory as the directories in owners, but actually we're expecting the file paths to be relative to the repository root, right? By changing the example, this could potentially be made clearer, and potentially better-test the functionality in generate_email_list. I think that for MockHost, with MockSCM (https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...), the root of the repository may be /mock-checkout/, and the layout test dir may be /mock-checkout/third_party/WebKit/LayoutTests/.
Description was changed from ========== Implement functionality to cc relevant owners. ========== to ========== Implement functionality to cc relevant owners. BUG=632798 ==========
https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:98: self.print_('No Failures, landing patch.') On 2016/07/29 at 17:42:19, qyearsley wrote: > Note: I believe committing means the same thing as landing in this context, so this could be kept as "committing" if you want. done https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:310: Turns the output from git cl try-results into a usable format. On 2016/07/29 at 17:42:19, qyearsley wrote: > This takes the output from `git diff master --name-only`, right? changed https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:314: point in the import process. On 2016/07/29 at 17:42:19, qyearsley wrote: > Nit: This wording may make it sound like it's a list of strings rather than a string with newline-separated file paths -- this might be clearer if you said something like "A string with newline-separated file paths relative to the repository root". done https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:315: directory_dict: A mapping of directories in imported to the On 2016/07/29 at 17:42:19, qyearsley wrote: > To make this clearer: > "in imported" -> "in the LayoutTests/imported directory" done https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:316: email address of the point of contact of it. On 2016/07/29 at 17:42:19, qyearsley wrote: > "of it" can be removed. done https://codereview.chromium.org/2188733003/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:320: import. On 2016/07/29 at 17:42:20, qyearsley wrote: > Nit: extra period. done
LGTM (with minor nit suggestions) :-) I think this looks good to commit after the debugging print statements are removed. https://codereview.chromium.org/2188733003/diff/160001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2188733003/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:308: """Generates a list of emails to be cc'd for current import Nit: missing period. https://codereview.chromium.org/2188733003/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:311: directories and generates list of contact emails. Nit: Extra space before "list". https://codereview.chromium.org/2188733003/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:315: to the repository root Nit: missing period. https://codereview.chromium.org/2188733003/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:334: print test_path Remember to remove debugging print statements.
The CQ bit was checked by raikiri@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2188733003/#ps180001 (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 #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement functionality to cc relevant owners. BUG=632798 ========== to ========== Implement functionality to cc relevant owners. BUG=632798 Committed: https://crrev.com/cbef1bb50c86c0b7da771e346d2ed38318289867 Cr-Commit-Position: refs/heads/master@{#409102} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cbef1bb50c86c0b7da771e346d2ed38318289867 Cr-Commit-Position: refs/heads/master@{#409102} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
