diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index e30f0d765..cdb0043ee 100644 --- a/bot/code_review_bot/__init__.py +++ b/bot/code_review_bot/__init__.py @@ -71,6 +71,37 @@ def is_publishable(self): raise Exception('Unsupported publication mode {}'.format(settings.publication)) + def as_diff(self): + ''' + Outputs as a diff block + ''' + assert self.diff is not None, 'Missing diff source' + fmt = '@@ -{line},{nb_minus} +{line},{nb_plus} @@\n{diff}' + + # Count the number of +/- + counts = {'common': 0, 'plus': 0, 'minus': 0} + clean_diff = [] + for line in self.diff.splitlines(): + if not line or line[0] == ' ': + key = 'common' + elif line[0] == '+': + key = 'plus' + elif line[0] == '-': + key = 'minus' + else: + # Skip invalid lines (like stderr output) + continue + + counts[key] += 1 + clean_diff.append(line) + + return fmt.format( + line=self.line, + diff='\n'.join(clean_diff), + nb_plus=counts['common'] + counts['plus'], + nb_minus=counts['common'] + counts['minus'], + ) + @abc.abstractmethod def validates(self): ''' diff --git a/bot/code_review_bot/report/phabricator.py b/bot/code_review_bot/report/phabricator.py index da51f63cf..bec15e51e 100644 --- a/bot/code_review_bot/report/phabricator.py +++ b/bot/code_review_bot/report/phabricator.py @@ -13,6 +13,7 @@ from code_review_bot import stats from code_review_bot.report.base import Reporter from code_review_bot.revisions import Revision +from code_review_bot.tasks.lint import MozLintIssue MODE_COMMENT = 'comment' MODE_HARBORMASTER = 'harbormaster' @@ -151,6 +152,12 @@ def comment_inline(self, revision, issue, existing_comments=[]): logger.warn('Will not publish inline comment on invalid path {}: {}'.format(issue.path, issue)) return + # Skip mozlint publication when a diff is available + # These issues are listed in an improvement patch + if isinstance(issue, MozLintIssue) and issue.diff is not None: + logger.info('Will not publish inline comment on formatting change: {}'.format(issue)) + return + # Check if comment is already posted comment = { 'diffID': revision.diff_id, diff --git a/bot/code_review_bot/revisions.py b/bot/code_review_bot/revisions.py index a8acfac8c..af7125ceb 100644 --- a/bot/code_review_bot/revisions.py +++ b/bot/code_review_bot/revisions.py @@ -168,7 +168,7 @@ def contains(self, issue): # Get modified lines for this issue modified_lines = self.lines.get(issue.path) if modified_lines is None: - logger.warn('Issue path is not in revision', path=issue.path, revision=self) + logger.debug('Issue path is not in revision', path=issue.path, revision=self) return False # Detect if this issue is in the patch diff --git a/bot/code_review_bot/tasks/base.py b/bot/code_review_bot/tasks/base.py index eac6e733f..c1e124672 100644 --- a/bot/code_review_bot/tasks/base.py +++ b/bot/code_review_bot/tasks/base.py @@ -98,10 +98,11 @@ def clean_path(self, path): path = path[1:] return path - def build_patches(self, artifacts): + def build_patches(self, artifacts, issues): ''' Some analyzers can provide a patch appliable by developers These patches are stored as Taskcluster artifacts and reported to developpers + They can also be built from issues produced earlier by the bot Output is a list of tuple (patch name as str, patch content as str) ''' return [] diff --git a/bot/code_review_bot/tasks/clang_format.py b/bot/code_review_bot/tasks/clang_format.py index 4bde6aab2..38fecc8d4 100644 --- a/bot/code_review_bot/tasks/clang_format.py +++ b/bot/code_review_bot/tasks/clang_format.py @@ -139,7 +139,7 @@ def parse_issues(self, artifacts, revision): for issue in issues ] - def build_patches(self, artifacts): + def build_patches(self, artifacts, issues): artifact = artifacts.get('public/code-review/clang-format.diff') if artifact is None: logger.warn('Missing or empty clang-format.diff') diff --git a/bot/code_review_bot/tasks/lint.py b/bot/code_review_bot/tasks/lint.py index cb22655b6..0052e6f0b 100644 --- a/bot/code_review_bot/tasks/lint.py +++ b/bot/code_review_bot/tasks/lint.py @@ -1,4 +1,7 @@ # -*- coding: utf-8 -*- +import itertools +from datetime import datetime + import structlog from libmozdata.phabricator import LintResult @@ -28,8 +31,7 @@ class MozLintIssue(Issue): ANALYZER = MOZLINT - def __init__(self, path, column, level, lineno, linter, message, rule, revision, **kwargs): - self.nb_lines = 1 + def __init__(self, path, column, level, lineno, linter, message, rule, revision, diff=None): self.column = column self.level = level self.line = lineno and int(lineno) or 0 # mozlint sometimes produce strings here @@ -38,13 +40,22 @@ def __init__(self, path, column, level, lineno, linter, message, rule, revision, self.rule = rule self.revision = revision self.path = path + self.diff = diff + + # Calc number of lines from patch when available + if isinstance(self.diff, str): + lines = self.diff.splitlines() + self.nb_lines = len(lines) + else: + self.nb_lines = 1 def __str__(self): return '{} issue {} {} line {}'.format( self.linter, self.level, self.path, - self.line, + # Display line range when multiple lines are in patch + '{}-{}'.format(self.line, self.line + self.nb_lines) if self.nb_lines > 1 else self.line, ) def build_extra_identifiers(self): @@ -75,8 +86,9 @@ def validates(self): A mozlint issues is publishable when: * file is not 3rd party * rule is not disabled + * issues without diff (those are published through a patch) ''' - return not self.is_third_party() and not self.is_disabled_rule() + return not self.is_third_party() and not self.is_disabled_rule() and self.diff is None def as_text(self): ''' @@ -178,8 +190,50 @@ def parse_issues(self, artifacts, revision): linter=issue['linter'], message=issue['message'], rule=issue['rule'], + diff=issue.get('diff'), ) for artifact in artifacts.values() for path, path_issues in artifact.items() for issue in path_issues ] + + def build_patches(self, artifacts, issues): + ''' + Build an improvement patch from issues with diff + Any issue on a file in patch will be posted + ''' + diff_issues = [ + i + for i in issues + if i.revision.has_file(i.path) and i.diff is not None + ] + if not diff_issues: + return [] + + header_fmt = '--- {path}\t{date}\n+++ {path}\t{date}\n' + + # Group issues by path + patch = '' + for path, path_issues in itertools.groupby(diff_issues, lambda i: i.path): + + if patch: + patch += '\n' + + # Add header for path + patch += header_fmt.format( + date=datetime.utcnow(), + path=path, + ) + + # Add each diff block, avoiding duplicates + # sorted by top line + chunks = [] + for issue in path_issues: + chunk = (issue.line, issue.as_diff()) + if chunk not in chunks: + chunks.append(chunk) + patch += '\n'.join(c[1] for c in sorted(chunks, key=lambda c: c[0])) + + return [ + ('mozlint', patch), + ] diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 4671a42be..5856adf7a 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -236,7 +236,7 @@ def _in_group(dep_id): logger.info('Found {} issues'.format(len(task_issues)), task=task.name, id=task.id) issues += task_issues - for name, patch in task.build_patches(artifacts): + for name, patch in task.build_patches(artifacts, task_issues): revision.add_improvement_patch(name, patch) except Exception as e: logger.warn('Failure during task analysis', task=settings.taskcluster.task_id, error=e)