From 4d30a65e095304a4f020abef24dbcd063991ad8a Mon Sep 17 00:00:00 2001 From: David Janke Date: Tue, 19 Apr 2016 13:04:43 -0600 Subject: [PATCH 01/14] move logic to get names of changed files into the base class --- eventhandler.py | 12 ++++++++++++ handlers/watchers/__init__.py | 22 ++++++++-------------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index 3ec77a9..d44aa87 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -45,6 +45,18 @@ def is_open_pr(self, payload): 'pull_request' in payload['issue']) + def get_changed_files(self, api): + changed_files = [] + diff = api.get_diff() + for line in diff.split('\n'): + if line.startswith('diff --git'): + changed_files.extend(line.split('diff --git ')[-1].split(' ')) + + # Remove the `a/` and `b/` parts of paths, + # And get unique values using `set()` + return set(map(lambda f: f if f.startswith('/') else f[2:], changed_files)) + + def reset_test_state(): global _warnings _warnings = [] diff --git a/handlers/watchers/__init__.py b/handlers/watchers/__init__.py index 0f47284..35ce330 100644 --- a/handlers/watchers/__init__.py +++ b/handlers/watchers/__init__.py @@ -20,20 +20,14 @@ def build_message(mentions): class WatchersHandler(EventHandler): def on_pr_opened(self, api, payload): user = payload['pull_request']['user']['login'] - diff = api.get_diff() - changed_files = [] - for line in diff.split('\n'): - if line.startswith('diff --git'): - changed_files.extend(line.split('diff --git ')[-1].split(' ')) - - # Remove the `a/` and `b/` parts of paths, - # And get unique values using `set()` - changed_files = set(map(lambda f: f if f.startswith('/') else f[2:], - changed_files)) - - watchers = get_people_from_config(api, WATCHERS_CONFIG_FILE) - if not watchers: - return + config = get_config() + changed_files = self.get_changed_files(api) + + repo = api.owner + '/' + api.repo + try: + watchers = config.items(repo) + except ConfigParser.NoSectionError: + return # No watchers mentions = defaultdict(list) for (watcher, watched_files) in watchers: From ee6356bf1b682d8ae0c95da809c30197bd6da9fe Mon Sep 17 00:00:00 2001 From: David Janke Date: Tue, 19 Apr 2016 17:56:04 -0600 Subject: [PATCH 02/14] move logic that finds "git diff" headers into the base class --- eventhandler.py | 14 ++++++++++---- handlers/missing_test/__init__.py | 17 ++++++++--------- handlers/no_modify_css_tests/__init__.py | 4 ++-- handlers/nonini_wpt_meta/__init__.py | 11 ++++------- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index d44aa87..940122f 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -13,6 +13,8 @@ 'labeled': 'on_issue_labeled' } +DIFF_HEADER_LINE_START = 'diff --git ' + class EventHandler: def on_pr_opened(self, api, payload): @@ -45,12 +47,16 @@ def is_open_pr(self, payload): 'pull_request' in payload['issue']) - def get_changed_files(self, api): - changed_files = [] + def get_diff_headers(self, api): diff = api.get_diff() for line in diff.split('\n'): - if line.startswith('diff --git'): - changed_files.extend(line.split('diff --git ')[-1].split(' ')) + if line.startswith(DIFF_HEADER_LINE_START): + yield line + + def get_changed_files(self, api): + changed_files = [] + for line in self.get_diff_headers(api): + changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) # Remove the `a/` and `b/` parts of paths, # And get unique values using `set()` diff --git a/handlers/missing_test/__init__.py b/handlers/missing_test/__init__.py index b4aa0e5..ad0a930 100644 --- a/handlers/missing_test/__init__.py +++ b/handlers/missing_test/__init__.py @@ -6,23 +6,22 @@ ' Please consider adding a test!') + class MissingTestHandler(EventHandler): COMPONENT_DIRS_TO_CHECK = ('layout', 'script', 'gfx', 'style', 'net') TEST_DIRS_TO_CHECK = ('ref', 'wpt', 'unit') def on_pr_opened(self, api, payload): - diff = api.get_diff() components_changed = set() - for line in diff.split('\n'): - if line.startswith('diff --git'): - for component in self.COMPONENT_DIRS_TO_CHECK: - if 'components/{0}/'.format(component) in line: - components_changed.add(component) + for line in self.get_diff_headers(api): + for component in self.COMPONENT_DIRS_TO_CHECK: + if 'components/{0}/'.format(component) in line: + components_changed.add(component) - for directory in self.TEST_DIRS_TO_CHECK: - if 'tests/{0}'.format(directory) in line: - return + for directory in self.TEST_DIRS_TO_CHECK: + if 'tests/{0}'.format(directory) in line: + return if components_changed: # Build a readable list of changed components diff --git a/handlers/no_modify_css_tests/__init__.py b/handlers/no_modify_css_tests/__init__.py index 0d37535..7054e5b 100644 --- a/handlers/no_modify_css_tests/__init__.py +++ b/handlers/no_modify_css_tests/__init__.py @@ -11,8 +11,8 @@ class NoModifyCSSTestsHandler(EventHandler): DIR_TO_CHECK = "tests/wpt/css-tests" def on_pr_opened(self, api, payload): - for line in api.get_diff().split('\n'): - if line.startswith("diff --git") and self.DIR_TO_CHECK in line: + for line in self.get_diff_headers(api): + if self.DIR_TO_CHECK in line: self.warn(NO_MODIFY_CSS_TESTS_MSG) break diff --git a/handlers/nonini_wpt_meta/__init__.py b/handlers/nonini_wpt_meta/__init__.py index 6e7ee19..c2d83e0 100644 --- a/handlers/nonini_wpt_meta/__init__.py +++ b/handlers/nonini_wpt_meta/__init__.py @@ -18,19 +18,16 @@ class NonINIWPTMetaFileHandler(EventHandler): 'mozilla-sync', ) - def _wpt_ini_dirs(self, line): - if line.startswith('diff --git') and '.' in line \ - and not any(fp in line for fp in self.FALSE_POSITIVE_SUBSTRINGS): - return set(directory for directory in self.DIRS_TO_CHECK - if directory in line) + def _wpt_ini_dirs(self, diff_line): + if '.' in diff_line and not any(fp in diff_line for fp in self.FALSE_POSITIVE_SUBSTRINGS): + return set(directory for directory in self.DIRS_TO_CHECK if directory in diff_line) else: return set() def on_pr_opened(self, api, payload): - diff = api.get_diff() test_dirs_with_offending_files = set() - for line in diff.split('\n'): + for line in self.get_diff_headers(api): test_dirs_with_offending_files |= self._wpt_ini_dirs(line) if test_dirs_with_offending_files: From 2a20dada15b416e023621e1d01ea23321522d03b Mon Sep 17 00:00:00 2001 From: David Janke Date: Tue, 19 Apr 2016 18:06:20 -0600 Subject: [PATCH 03/14] move logic that finds added lines into the base class --- eventhandler.py | 7 +++++++ handlers/unsafe/__init__.py | 6 +++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index 940122f..cb4a2fa 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -53,6 +53,13 @@ def get_diff_headers(self, api): if line.startswith(DIFF_HEADER_LINE_START): yield line + def get_added_lines(self, api): + diff = api.get_diff() + for line in diff.split('\n'): + if line.startswith('+') and not line.startswith('+++'): + # prefix of one or two pluses (+) + yield line + def get_changed_files(self, api): changed_files = [] for line in self.get_diff_headers(api): diff --git a/handlers/unsafe/__init__.py b/handlers/unsafe/__init__.py index 09ce152..96010ea 100644 --- a/handlers/unsafe/__init__.py +++ b/handlers/unsafe/__init__.py @@ -8,11 +8,11 @@ 'Please review it carefully!') + class UnsafeHandler(EventHandler): def on_pr_opened(self, api, payload): - diff = api.get_diff() - for line in diff.split('\n'): - if is_addition(line) and line.find('unsafe ') > -1: + for line in self.get_added_lines(api): + if line.find('unsafe ') > -1: self.warn(unsafe_warning_msg) return From 161d9398559e403dcae4274297191acf5bc03d29 Mon Sep 17 00:00:00 2001 From: David Janke Date: Tue, 19 Apr 2016 18:17:00 -0600 Subject: [PATCH 04/14] replace lambda with list comprehension --- eventhandler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eventhandler.py b/eventhandler.py index cb4a2fa..9f9d3b4 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -67,7 +67,7 @@ def get_changed_files(self, api): # Remove the `a/` and `b/` parts of paths, # And get unique values using `set()` - return set(map(lambda f: f if f.startswith('/') else f[2:], changed_files)) + return set(f if f.startswith('/') else f[2:] for f in changed_files) def reset_test_state(): From bc2399d7409bb37f74164d364d8e9735d23e6aea Mon Sep 17 00:00:00 2001 From: David Janke Date: Wed, 20 Apr 2016 13:12:50 -0600 Subject: [PATCH 05/14] use splitlines() for more robust line break detection --- eventhandler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index 9f9d3b4..e8044b7 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -49,13 +49,13 @@ def is_open_pr(self, payload): def get_diff_headers(self, api): diff = api.get_diff() - for line in diff.split('\n'): + for line in diff.splitlines(): if line.startswith(DIFF_HEADER_LINE_START): yield line def get_added_lines(self, api): diff = api.get_diff() - for line in diff.split('\n'): + for line in diff.splitlines(): if line.startswith('+') and not line.startswith('+++'): # prefix of one or two pluses (+) yield line From 70620b73b1b41055632724fad1c72942ebf1acd4 Mon Sep 17 00:00:00 2001 From: David Janke Date: Thu, 21 Apr 2016 21:56:13 -0600 Subject: [PATCH 06/14] factor out logic to strip test directories from file path --- eventhandler.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index e8044b7..5c5403c 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -12,6 +12,7 @@ 'closed': 'on_pr_closed', 'labeled': 'on_issue_labeled' } +_test_path_roots = ['a/', 'b/'] DIFF_HEADER_LINE_START = 'diff --git ' @@ -65,9 +66,16 @@ def get_changed_files(self, api): for line in self.get_diff_headers(api): changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) - # Remove the `a/` and `b/` parts of paths, # And get unique values using `set()` - return set(f if f.startswith('/') else f[2:] for f in changed_files) + return set(f for f in map(self.normalize_file_path, changed_files) if f is not None) + + def normalize_file_path(self, filepath): + if filepath is None or filepath.strip() == '': + return None + for prefix in _test_path_roots: + if filepath.startswith(prefix): + return filepath[len(prefix):] + return filepath def reset_test_state(): @@ -83,8 +91,8 @@ def get_warnings(): def get_handlers(): modules = [] handlers = [] - possiblehandlers = os.listdir('handlers') - for i in possiblehandlers: + possible_handlers = os.listdir('handlers') + for i in possible_handlers: location = os.path.join('handlers', i) try: module = imp.load_module('handlers.' + i, None, location, From 65a40ed5ebf2acff6e91ebd22c8fd536f1e35b56 Mon Sep 17 00:00:00 2001 From: David Janke Date: Wed, 11 May 2016 12:37:42 -0600 Subject: [PATCH 07/14] fix incorrect merge --- handlers/watchers/__init__.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/handlers/watchers/__init__.py b/handlers/watchers/__init__.py index 35ce330..554572c 100644 --- a/handlers/watchers/__init__.py +++ b/handlers/watchers/__init__.py @@ -23,11 +23,9 @@ def on_pr_opened(self, api, payload): config = get_config() changed_files = self.get_changed_files(api) - repo = api.owner + '/' + api.repo - try: - watchers = config.items(repo) - except ConfigParser.NoSectionError: - return # No watchers + watchers = get_people_from_config(api, WATCHERS_CONFIG_FILE) + if not watchers: + return mentions = defaultdict(list) for (watcher, watched_files) in watchers: From 5d53abecd47f55b22f62a7423bbe236b8e4b6946 Mon Sep 17 00:00:00 2001 From: David Janke Date: Wed, 11 May 2016 13:08:50 -0600 Subject: [PATCH 08/14] move diff parsing logic into APIProvider --- eventhandler.py | 33 --------------------- handlers/missing_test/__init__.py | 2 +- handlers/no_modify_css_tests/__init__.py | 2 +- handlers/nonini_wpt_meta/__init__.py | 2 +- handlers/unsafe/__init__.py | 2 +- handlers/watchers/__init__.py | 3 +- newpr.py | 37 ++++++++++++++++++++++++ 7 files changed, 42 insertions(+), 39 deletions(-) diff --git a/eventhandler.py b/eventhandler.py index 5c5403c..cfd91f4 100644 --- a/eventhandler.py +++ b/eventhandler.py @@ -12,9 +12,6 @@ 'closed': 'on_pr_closed', 'labeled': 'on_issue_labeled' } -_test_path_roots = ['a/', 'b/'] - -DIFF_HEADER_LINE_START = 'diff --git ' class EventHandler: @@ -48,36 +45,6 @@ def is_open_pr(self, payload): 'pull_request' in payload['issue']) - def get_diff_headers(self, api): - diff = api.get_diff() - for line in diff.splitlines(): - if line.startswith(DIFF_HEADER_LINE_START): - yield line - - def get_added_lines(self, api): - diff = api.get_diff() - for line in diff.splitlines(): - if line.startswith('+') and not line.startswith('+++'): - # prefix of one or two pluses (+) - yield line - - def get_changed_files(self, api): - changed_files = [] - for line in self.get_diff_headers(api): - changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) - - # And get unique values using `set()` - return set(f for f in map(self.normalize_file_path, changed_files) if f is not None) - - def normalize_file_path(self, filepath): - if filepath is None or filepath.strip() == '': - return None - for prefix in _test_path_roots: - if filepath.startswith(prefix): - return filepath[len(prefix):] - return filepath - - def reset_test_state(): global _warnings _warnings = [] diff --git a/handlers/missing_test/__init__.py b/handlers/missing_test/__init__.py index ad0a930..bc21847 100644 --- a/handlers/missing_test/__init__.py +++ b/handlers/missing_test/__init__.py @@ -14,7 +14,7 @@ class MissingTestHandler(EventHandler): def on_pr_opened(self, api, payload): components_changed = set() - for line in self.get_diff_headers(api): + for line in api.get_diff_headers(): for component in self.COMPONENT_DIRS_TO_CHECK: if 'components/{0}/'.format(component) in line: components_changed.add(component) diff --git a/handlers/no_modify_css_tests/__init__.py b/handlers/no_modify_css_tests/__init__.py index 7054e5b..0dffecd 100644 --- a/handlers/no_modify_css_tests/__init__.py +++ b/handlers/no_modify_css_tests/__init__.py @@ -11,7 +11,7 @@ class NoModifyCSSTestsHandler(EventHandler): DIR_TO_CHECK = "tests/wpt/css-tests" def on_pr_opened(self, api, payload): - for line in self.get_diff_headers(api): + for line in api.get_diff_headers(): if self.DIR_TO_CHECK in line: self.warn(NO_MODIFY_CSS_TESTS_MSG) break diff --git a/handlers/nonini_wpt_meta/__init__.py b/handlers/nonini_wpt_meta/__init__.py index c2d83e0..3d0af09 100644 --- a/handlers/nonini_wpt_meta/__init__.py +++ b/handlers/nonini_wpt_meta/__init__.py @@ -27,7 +27,7 @@ def _wpt_ini_dirs(self, diff_line): def on_pr_opened(self, api, payload): test_dirs_with_offending_files = set() - for line in self.get_diff_headers(api): + for line in api.get_diff_headers(): test_dirs_with_offending_files |= self._wpt_ini_dirs(line) if test_dirs_with_offending_files: diff --git a/handlers/unsafe/__init__.py b/handlers/unsafe/__init__.py index 96010ea..b658f4b 100644 --- a/handlers/unsafe/__init__.py +++ b/handlers/unsafe/__init__.py @@ -11,7 +11,7 @@ class UnsafeHandler(EventHandler): def on_pr_opened(self, api, payload): - for line in self.get_added_lines(api): + for line in api.get_added_lines(): if line.find('unsafe ') > -1: self.warn(unsafe_warning_msg) return diff --git a/handlers/watchers/__init__.py b/handlers/watchers/__init__.py index 554572c..65f129d 100644 --- a/handlers/watchers/__init__.py +++ b/handlers/watchers/__init__.py @@ -20,8 +20,7 @@ def build_message(mentions): class WatchersHandler(EventHandler): def on_pr_opened(self, api, payload): user = payload['pull_request']['user']['login'] - config = get_config() - changed_files = self.get_changed_files(api) + changed_files = api.get_changed_files() watchers = get_people_from_config(api, WATCHERS_CONFIG_FILE) if not watchers: diff --git a/newpr.py b/newpr.py index b0224b1..713374d 100755 --- a/newpr.py +++ b/newpr.py @@ -17,6 +17,10 @@ import eventhandler +_test_path_roots = ['a/', 'b/'] + +DIFF_HEADER_LINE_START = 'diff --git ' + class APIProvider(object): def __init__(self, payload, user): @@ -25,6 +29,7 @@ def __init__(self, payload, user): self.repo = repo self.issue = issue self.user = user + self.changed_files = None def is_new_contributor(self, username): raise NotImplementedError @@ -53,6 +58,38 @@ def get_pull(self): def get_page_content(self, url): raise NotImplementedError + def get_diff_headers(self): + diff = self.get_diff() + for line in diff.splitlines(): + if line.startswith(DIFF_HEADER_LINE_START): + yield line + + def get_changed_files(self): + if self.changed_files is None: + changed_files = [] + for line in self.get_diff_headers(): + changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) + + # And get unique values using `set()` + self.changed_files = set(f for f in map(APIProvider.normalize_file_path, changed_files) if f is not None) + return self.changed_files + + def get_added_lines(self): + diff = self.get_diff() + for line in diff.splitlines(): + if line.startswith('+') and not line.startswith('+++'): + # prefix of one or two pluses (+) + yield line + + @staticmethod + def normalize_file_path(filepath): + if filepath is None or filepath.strip() == '': + return None + for prefix in _test_path_roots: + if filepath.startswith(prefix): + return filepath[len(prefix):] + return filepath + class GithubAPIProvider(APIProvider): BASE_URL = "https://api.github.com/repos/" From bbaa281d787e24ec1403d989c6eeed31a35d8913 Mon Sep 17 00:00:00 2001 From: David Janke Date: Fri, 13 May 2016 12:25:21 -0600 Subject: [PATCH 09/14] use more specific "get_changed_files" method --- handlers/missing_test/__init__.py | 2 +- handlers/no_modify_css_tests/__init__.py | 2 +- handlers/nonini_wpt_meta/__init__.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/handlers/missing_test/__init__.py b/handlers/missing_test/__init__.py index bc21847..bd8af54 100644 --- a/handlers/missing_test/__init__.py +++ b/handlers/missing_test/__init__.py @@ -14,7 +14,7 @@ class MissingTestHandler(EventHandler): def on_pr_opened(self, api, payload): components_changed = set() - for line in api.get_diff_headers(): + for line in api.get_changed_files(): for component in self.COMPONENT_DIRS_TO_CHECK: if 'components/{0}/'.format(component) in line: components_changed.add(component) diff --git a/handlers/no_modify_css_tests/__init__.py b/handlers/no_modify_css_tests/__init__.py index 0dffecd..b033c2b 100644 --- a/handlers/no_modify_css_tests/__init__.py +++ b/handlers/no_modify_css_tests/__init__.py @@ -11,7 +11,7 @@ class NoModifyCSSTestsHandler(EventHandler): DIR_TO_CHECK = "tests/wpt/css-tests" def on_pr_opened(self, api, payload): - for line in api.get_diff_headers(): + for line in api.get_changed_files(): if self.DIR_TO_CHECK in line: self.warn(NO_MODIFY_CSS_TESTS_MSG) break diff --git a/handlers/nonini_wpt_meta/__init__.py b/handlers/nonini_wpt_meta/__init__.py index 3d0af09..b40739d 100644 --- a/handlers/nonini_wpt_meta/__init__.py +++ b/handlers/nonini_wpt_meta/__init__.py @@ -27,7 +27,7 @@ def _wpt_ini_dirs(self, diff_line): def on_pr_opened(self, api, payload): test_dirs_with_offending_files = set() - for line in api.get_diff_headers(): + for line in api.get_changed_files(): test_dirs_with_offending_files |= self._wpt_ini_dirs(line) if test_dirs_with_offending_files: From 06750b1d5d52959023cf184ed67202f851dfec75 Mon Sep 17 00:00:00 2001 From: David Janke Date: Thu, 9 Jun 2016 12:50:14 -0600 Subject: [PATCH 10/14] change "line" variables to "filepath" for results of "get_changed_files" --- handlers/missing_test/__init__.py | 6 +++--- handlers/no_modify_css_tests/__init__.py | 4 ++-- handlers/nonini_wpt_meta/__init__.py | 4 ++-- handlers/watchers/__init__.py | 9 ++++----- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/handlers/missing_test/__init__.py b/handlers/missing_test/__init__.py index bd8af54..a77749c 100644 --- a/handlers/missing_test/__init__.py +++ b/handlers/missing_test/__init__.py @@ -14,13 +14,13 @@ class MissingTestHandler(EventHandler): def on_pr_opened(self, api, payload): components_changed = set() - for line in api.get_changed_files(): + for filepath in api.get_changed_files(): for component in self.COMPONENT_DIRS_TO_CHECK: - if 'components/{0}/'.format(component) in line: + if 'components/{0}/'.format(component) in filepath: components_changed.add(component) for directory in self.TEST_DIRS_TO_CHECK: - if 'tests/{0}'.format(directory) in line: + if 'tests/{0}'.format(directory) in filepath: return if components_changed: diff --git a/handlers/no_modify_css_tests/__init__.py b/handlers/no_modify_css_tests/__init__.py index b033c2b..7d9bd80 100644 --- a/handlers/no_modify_css_tests/__init__.py +++ b/handlers/no_modify_css_tests/__init__.py @@ -11,8 +11,8 @@ class NoModifyCSSTestsHandler(EventHandler): DIR_TO_CHECK = "tests/wpt/css-tests" def on_pr_opened(self, api, payload): - for line in api.get_changed_files(): - if self.DIR_TO_CHECK in line: + for filepath in api.get_changed_files(): + if self.DIR_TO_CHECK in filepath: self.warn(NO_MODIFY_CSS_TESTS_MSG) break diff --git a/handlers/nonini_wpt_meta/__init__.py b/handlers/nonini_wpt_meta/__init__.py index b40739d..836bf86 100644 --- a/handlers/nonini_wpt_meta/__init__.py +++ b/handlers/nonini_wpt_meta/__init__.py @@ -27,8 +27,8 @@ def _wpt_ini_dirs(self, diff_line): def on_pr_opened(self, api, payload): test_dirs_with_offending_files = set() - for line in api.get_changed_files(): - test_dirs_with_offending_files |= self._wpt_ini_dirs(line) + for filepath in api.get_changed_files(): + test_dirs_with_offending_files |= self._wpt_ini_dirs(filepath) if test_dirs_with_offending_files: if len(test_dirs_with_offending_files) == 1: diff --git a/handlers/watchers/__init__.py b/handlers/watchers/__init__.py index 65f129d..fc9f308 100644 --- a/handlers/watchers/__init__.py +++ b/handlers/watchers/__init__.py @@ -20,7 +20,6 @@ def build_message(mentions): class WatchersHandler(EventHandler): def on_pr_opened(self, api, payload): user = payload['pull_request']['user']['login'] - changed_files = api.get_changed_files() watchers = get_people_from_config(api, WATCHERS_CONFIG_FILE) if not watchers: @@ -35,15 +34,15 @@ def on_pr_opened(self, api, payload): blacklisted_files.append(watched_file[1:]) for blacklisted_file in blacklisted_files: watched_files.remove('-' + blacklisted_file) - for changed_file in changed_files: + for filepath in api.get_changed_files(): for blacklisted_file in blacklisted_files: - if changed_file.startswith(blacklisted_file): + if filepath.startswith(blacklisted_file): break else: for watched_file in watched_files: - if (changed_file.startswith(watched_file) and + if (filepath.startswith(watched_file) and user != watcher): - mentions[watcher].append(changed_file) + mentions[watcher].append(filepath) if not mentions: return From 5b908089820ea4554e7c3f2bd1ddbaeb0ce28123 Mon Sep 17 00:00:00 2001 From: David Janke Date: Thu, 9 Jun 2016 12:57:52 -0600 Subject: [PATCH 11/14] use api helper method in EmptyTitleElementHandler --- handlers/empty_title_element/__init__.py | 7 ++----- helpers.py | 7 ------- newpr.py | 9 ++++++++- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/handlers/empty_title_element/__init__.py b/handlers/empty_title_element/__init__.py index a38185b..f854804 100644 --- a/handlers/empty_title_element/__init__.py +++ b/handlers/empty_title_element/__init__.py @@ -1,7 +1,5 @@ from __future__ import absolute_import - from eventhandler import EventHandler -from helpers import is_addition WARNING = ("These commits include an empty title element (``). " "Consider adding appropriate metadata.") @@ -9,9 +7,8 @@ class EmptyTitleElementHandler(EventHandler): def on_pr_opened(self, api, payload): - diff = api.get_diff() - for line in diff.split('\n'): - if is_addition(line) and line.find("") > -1: + for line in api.get_added_lines(): + if line.find("") > -1: # This test doesn't consider case and whitespace the same way # that a HTML parser does, so empty title elements might still # go unnoticed. It will catch the low-hanging fruit, though. diff --git a/helpers.py b/helpers.py index a84013f..a470787 100644 --- a/helpers.py +++ b/helpers.py @@ -23,13 +23,6 @@ def get_collaborators(api): return [username for (username, _) in config_items] -def is_addition(diff_line): - """ - Checks if a line from a unified diff is an addition. - """ - return diff_line.startswith('+') and not diff_line.startswith('+++') - - def linear_search(sequence, element, callback=lambda thing: thing): ''' The 'in' operator also does a linear search over a sequence, but it checks diff --git a/newpr.py b/newpr.py index 713374d..76fe103 100755 --- a/newpr.py +++ b/newpr.py @@ -77,10 +77,17 @@ def get_changed_files(self): def get_added_lines(self): diff = self.get_diff() for line in diff.splitlines(): - if line.startswith('+') and not line.startswith('+++'): + if self.is_addition(line): # prefix of one or two pluses (+) yield line + @staticmethod + def is_addition(diff_line): + """ + Checks if a line from a unified diff is an addition. + """ + return diff_line.startswith('+') and not diff_line.startswith('+++') + @staticmethod def normalize_file_path(filepath): if filepath is None or filepath.strip() == '': From 445373a96bafd9f838d8b19ea99123e772eefb71 Mon Sep 17 00:00:00 2001 From: David Janke Date: Thu, 9 Jun 2016 13:08:24 -0600 Subject: [PATCH 12/14] move static APIProvider methods into helpers module --- helpers.py | 23 +++++++++++++++++++++-- newpr.py | 23 +++-------------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/helpers.py b/helpers.py index a470787..335f82a 100644 --- a/helpers.py +++ b/helpers.py @@ -7,6 +7,9 @@ 'collaborators.ini') +_test_path_roots = ['a/', 'b/'] + + def get_people_from_config(api, config_abs_path): config = ConfigParser.ConfigParser() config.read(config_abs_path) @@ -23,13 +26,29 @@ def get_collaborators(api): return [username for (username, _) in config_items] +def is_addition(diff_line): + """ + Checks if a line from a unified diff is an addition. + """ + return diff_line.startswith('+') and not diff_line.startswith('+++') + + +def normalize_file_path(filepath): + if filepath is None or filepath.strip() == '': + return None + for prefix in _test_path_roots: + if filepath.startswith(prefix): + return filepath[len(prefix):] + return filepath + + def linear_search(sequence, element, callback=lambda thing: thing): - ''' + """ The 'in' operator also does a linear search over a sequence, but it checks for the exact match of the given object, whereas this makes use of '==', which calls the '__eq__' magic method, which could've been overridden in a custom class (which is the case for our test lint) - ''' + """ for thing in sequence: if element == thing: # element could have an overridden '__eq__' callback(thing) diff --git a/newpr.py b/newpr.py index 76fe103..36f97ed 100755 --- a/newpr.py +++ b/newpr.py @@ -16,8 +16,7 @@ import urllib2 import eventhandler - -_test_path_roots = ['a/', 'b/'] +from helpers import is_addition, normalize_file_path DIFF_HEADER_LINE_START = 'diff --git ' @@ -71,32 +70,16 @@ def get_changed_files(self): changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) # And get unique values using `set()` - self.changed_files = set(f for f in map(APIProvider.normalize_file_path, changed_files) if f is not None) + self.changed_files = set(f for f in map(normalize_file_path, changed_files) if f is not None) return self.changed_files def get_added_lines(self): diff = self.get_diff() for line in diff.splitlines(): - if self.is_addition(line): + if is_addition(line): # prefix of one or two pluses (+) yield line - @staticmethod - def is_addition(diff_line): - """ - Checks if a line from a unified diff is an addition. - """ - return diff_line.startswith('+') and not diff_line.startswith('+++') - - @staticmethod - def normalize_file_path(filepath): - if filepath is None or filepath.strip() == '': - return None - for prefix in _test_path_roots: - if filepath.startswith(prefix): - return filepath[len(prefix):] - return filepath - class GithubAPIProvider(APIProvider): BASE_URL = "https://api.github.com/repos/" From a3b446d1011e26a90f6c6ddbcffd7a5e1f257a5e Mon Sep 17 00:00:00 2001 From: David Janke Date: Thu, 9 Jun 2016 13:20:32 -0600 Subject: [PATCH 13/14] add docstring to helper method --- helpers.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/helpers.py b/helpers.py index 335f82a..2d80344 100644 --- a/helpers.py +++ b/helpers.py @@ -34,8 +34,13 @@ def is_addition(diff_line): def normalize_file_path(filepath): + """ + Strip any leading/training whitespace. + Remove any test directories from the start of the path + """ if filepath is None or filepath.strip() == '': return None + filepath = filepath.strip() for prefix in _test_path_roots: if filepath.startswith(prefix): return filepath[len(prefix):] From 4abf6b36ef3ecf15d044e859ff9103ef3feebd10 Mon Sep 17 00:00:00 2001 From: David Janke Date: Fri, 10 Jun 2016 08:18:58 -0600 Subject: [PATCH 14/14] fix linter errors --- handlers/missing_test/__init__.py | 1 - handlers/nonini_wpt_meta/__init__.py | 8 +++++--- handlers/unsafe/__init__.py | 2 -- newpr.py | 10 ++++++---- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/handlers/missing_test/__init__.py b/handlers/missing_test/__init__.py index a77749c..7c86078 100644 --- a/handlers/missing_test/__init__.py +++ b/handlers/missing_test/__init__.py @@ -6,7 +6,6 @@ ' Please consider adding a test!') - class MissingTestHandler(EventHandler): COMPONENT_DIRS_TO_CHECK = ('layout', 'script', 'gfx', 'style', 'net') TEST_DIRS_TO_CHECK = ('ref', 'wpt', 'unit') diff --git a/handlers/nonini_wpt_meta/__init__.py b/handlers/nonini_wpt_meta/__init__.py index 836bf86..4a6ad20 100644 --- a/handlers/nonini_wpt_meta/__init__.py +++ b/handlers/nonini_wpt_meta/__init__.py @@ -18,9 +18,11 @@ class NonINIWPTMetaFileHandler(EventHandler): 'mozilla-sync', ) - def _wpt_ini_dirs(self, diff_line): - if '.' in diff_line and not any(fp in diff_line for fp in self.FALSE_POSITIVE_SUBSTRINGS): - return set(directory for directory in self.DIRS_TO_CHECK if directory in diff_line) + def _wpt_ini_dirs(self, line): + if '.' in line and not any(fp in line + for fp in self.FALSE_POSITIVE_SUBSTRINGS): + return set(directory for directory in self.DIRS_TO_CHECK + if directory in line) else: return set() diff --git a/handlers/unsafe/__init__.py b/handlers/unsafe/__init__.py index b658f4b..ddd5135 100644 --- a/handlers/unsafe/__init__.py +++ b/handlers/unsafe/__init__.py @@ -1,14 +1,12 @@ from __future__ import absolute_import from eventhandler import EventHandler -from helpers import is_addition unsafe_warning_msg = ('These commits modify **unsafe code**. ' 'Please review it carefully!') - class UnsafeHandler(EventHandler): def on_pr_opened(self, api, payload): for line in api.get_added_lines(): diff --git a/newpr.py b/newpr.py index 36f97ed..9cd440f 100755 --- a/newpr.py +++ b/newpr.py @@ -18,7 +18,7 @@ import eventhandler from helpers import is_addition, normalize_file_path -DIFF_HEADER_LINE_START = 'diff --git ' +DIFF_HEADER_PREFIX = 'diff --git ' class APIProvider(object): @@ -60,17 +60,19 @@ def get_page_content(self, url): def get_diff_headers(self): diff = self.get_diff() for line in diff.splitlines(): - if line.startswith(DIFF_HEADER_LINE_START): + if line.startswith(DIFF_HEADER_PREFIX): yield line def get_changed_files(self): if self.changed_files is None: changed_files = [] for line in self.get_diff_headers(): - changed_files.extend(line.split(DIFF_HEADER_LINE_START)[-1].split(' ')) + files = line.split(DIFF_HEADER_PREFIX)[-1].split(' ') + changed_files.extend(files) # And get unique values using `set()` - self.changed_files = set(f for f in map(normalize_file_path, changed_files) if f is not None) + normalized = map(normalize_file_path, changed_files) + self.changed_files = set(f for f in normalized if f is not None) return self.changed_files def get_added_lines(self):