Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First pass at Swarm interface integration for inline comments #193

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
4 changes: 3 additions & 1 deletion inlineplz/interfaces/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
from __future__ import unicode_literals

from inlineplz.interfaces.github import GitHubInterface
from inlineplz.interfaces.swarm import SwarmInterface

INTERFACES = {
'github': GitHubInterface
'github': GitHubInterface,
'swarm': SwarmInterface
}
48 changes: 38 additions & 10 deletions inlineplz/interfaces/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@

import github3
import unidiff
import giturlparse

from inlineplz.interfaces.base import InterfaceBase
from inlineplz.util import git, system


class GitHubInterface(InterfaceBase):
def __init__(self, owner, repo, pr=None, branch=None, token=None, url=None):
def __init__(self, args):
"""
GitHubInterface lets us post messages to GitHub.

Expand All @@ -28,6 +29,32 @@ def __init__(self, owner, repo, pr=None, branch=None, token=None, url=None):

url is the base URL of your GitHub instance, such as https://github.com
"""
url = args.url
if args.repo_slug:
owner = args.repo_slug.split('/')[0]
repo = args.repo_slug.split('/')[1]
else:
owner = args.owner
repo = args.repo
if args.url:
try:
url_to_parse = args.url
# giturlparse won't parse URLs that don't end in .git
if not url_to_parse.endswith('.git'):
url_to_parse += '.git'
parsed = giturlparse.parse(url_to_parse)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you need to add an import for giturlparse in this file

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and delete the giturlparse import from main.py)

url = parsed.resource
if not url.startswith('https://'):
url = 'https://' + url
owner = parsed.owner
repo = parsed.name
except giturlparse.parser.ParserError:
pass

pr = args.pull_request
token = args.token
branch = args.branch

self.github = None
if not url or url == 'https://github.com':
self.github = github3.GitHub(token=token)
Expand Down Expand Up @@ -89,35 +116,36 @@ def post_messages(self, messages, max_comments):
return messages_to_post
if not msg.comments:
continue
msg_position = self.position(msg)
msg_path = os.path.relpath(msg.path).replace('\\', '/').strip()
msg_position = self.position(msg, msg_path)
if msg_position:
messages_to_post += 1
if not self.is_duplicate(msg, msg_position):
if not self.is_duplicate(msg, msg_path, msg_position):
# skip this message if we already have too many comments on this file
# max comments / 5 is an arbitrary number i totally made up. should maybe be configurable.
if paths.setdefault(msg.path, 0) > max_comments // 5:
if paths.setdefault(msg_path, 0) > max_comments // 5:
continue
try:
print('Creating review comment: {0}'.format(msg))
self.pull_request.create_review_comment(
self.format_message(msg),
self.last_sha,
msg.path,
msg_path,
msg_position
)
except github3.GitHubError:
pass
paths[msg.path] += 1
paths[msg_path] += 1
messages_posted += 1
if max_comments >= 0 and messages_posted > max_comments:
break
print('{} messages posted to Github.'.format(messages_to_post))
return messages_to_post

def is_duplicate(self, message, position):
def is_duplicate(self, message, path, position):
for comment in self.pull_request.review_comments():
if (comment.position == position and
comment.path == message.path and
comment.path == path and
comment.body.strip() == self.format_message(message).strip()):
return True
return False
Expand All @@ -134,14 +162,14 @@ def format_message(message):
)
return '`{0}`'.format(list(message.comments)[0].strip())

def position(self, message):
def position(self, message, path):
"""Calculate position within the PR, which is not the line number"""
if not message.line_number:
message.line_number = 1
patch = unidiff.PatchSet(self.diff.split('\n'))
for patched_file in patch:
target = patched_file.target_file.lstrip('b/')
if target == message.path:
if target == path:
offset = 1
for hunk_no, hunk in enumerate(patched_file):
for position, hunk_line in enumerate(hunk):
Expand Down
101 changes: 101 additions & 0 deletions inlineplz/interfaces/swarm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# -*- coding: utf-8 -*-

import random
import subprocess

import requests

from inlineplz.interfaces.base import InterfaceBase

class SwarmInterface(InterfaceBase):
def __init__(self, args):
"""
SwarmInterface lets us post messages to Swarm (Helix).

username and password are the credentials used to access Swarm/Perforce.
host is the server (And any additional paths before the api)
topic is the the review you are commenting on (for reviews, it will typically be "review/###" for some review number)
"""
self.username = args.username
self.password = args.password
self.host = args.host
self.topic = args.topic
# current implementation uses version 8 of the implementation
# https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Swarm_API%3FTocPath%3DSwarm%2520API%7C_____0
self.version = 'v8'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this version come from? can we document that?


def post_messages(self, messages, max_comments):
# randomize message order to more evenly distribute messages across different files
messages = list(messages)
random.shuffle(messages)

messages_to_post = 0
messages_posted = 0
current_comments = self.get_comments(max_comments)
for msg in messages:
if not msg.comments:
continue
messages_to_post += 1
body = self.format_message(msg)
proc = subprocess.Popen(["p4", "fstat", "-T", "depotFile", msg.path], stdout=subprocess.PIPE)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer if we used subprocess.check_output unless theres a good reason to do it this way.

output = proc.stdout.read()
l = output.split()
if (len(l) != 3):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary parens

print("Can't find depotFile for '{}': {}".format(msg.path, output))
continue
path = output.split()[2]
if self.is_duplicate(current_comments, body, path, msg.line_number):
print("Duplicate for {}:{}".format(path, msg.line_number))
continue
# try to send swarm post comment
self.post_comment(body, path, msg.line_number)
messages_posted += 1
if max_comments >= 0 and messages_posted > max_comments:
break
print('{} messages posted to Swarm.'.format(messages_to_post))
return messages_to_post

def post_comment(self, body, path, line_number):
# https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3
url = "https://{}/api/{}/comments".format(self.host, self.version)
payload = {
'topic': self.topic,
'body': body,
'context[file]': path,
'context[rightLine]': line_number
}
print("".format(payload))
requests.post(url, auth=(self.username, self.password), data=payload)

def get_comments(self, max_comments=100):
# https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3
parameters = "topic={}&max={}".format(self.topic, max_comments)
url = "https://{}/api/{}/comments?{}".format(self.host, self.version, parameters)
response = requests.get(url, auth=(self.username, self.password))
if (response.status_code != requests.codes.ok):
print("Can't get comments, status code: {}".format(response.status_code))
return {}
return response.json()["comments"]

@staticmethod
def is_duplicate(comments, body, path, line_number):
for comment in comments:
try:
if (comment["context"]["rightLine"] == line_number and
comment["context"]["file"] == path and
comment["body"].strip() == body.strip()):
print("Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"]))
return True
except (KeyError, TypeError):
continue
return False

@staticmethod
def format_message(message):
if not message.comments:
return ''
return (
'```\n' +
'\n'.join(sorted(list(message.comments))) +
'\n```'
)
44 changes: 9 additions & 35 deletions inlineplz/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import sys
import time

import giturlparse
import yaml

from inlineplz import interfaces
Expand All @@ -21,14 +20,18 @@

def main():
parser = argparse.ArgumentParser()
parser.add_argument('--pull-request', type=int)
parser.add_argument('--pull-request', type=int, default=None)
parser.add_argument('--owner', type=str)
parser.add_argument('--repo', type=str)
parser.add_argument('--repo-slug', type=str)
parser.add_argument('--branch', type=str)
parser.add_argument('--token', type=str)
parser.add_argument('--branch', type=str, default=None)
parser.add_argument('--token', type=str, default=None)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need some sort of auth for p4?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and/or for swarm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's in the username and password.

parser.add_argument('--interface', type=str, choices=interfaces.INTERFACES)
parser.add_argument('--url', type=str)
parser.add_argument('--url', type=str, default=None)
parser.add_argument('--username', type=str)
parser.add_argument('--password', type=str)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we somehow combine password / token into a single auth arg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can do that.

parser.add_argument('--host', type=str)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is host different from url? could we use the same arg for both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a url, it's just the hostname (plus extra directories if needed). We could use a url as a base?

parser.add_argument('--topic', type=str)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we rename this to something like review-id? and deprecate pull-request?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could do review-id, I like that.

parser.add_argument('--enabled-linters', type=str, nargs='+')
parser.add_argument('--disabled-linters', type=str, nargs='+')
parser.add_argument('--dryrun', action='store_true')
Expand Down Expand Up @@ -120,28 +123,6 @@ def inline(args):
trusted = args.trusted
args = load_config(args)

# TODO: consider moving this git parsing stuff into the github interface
url = args.url
if args.repo_slug:
owner = args.repo_slug.split('/')[0]
repo = args.repo_slug.split('/')[1]
else:
owner = args.owner
repo = args.repo
if args.url:
try:
url_to_parse = args.url
# giturlparse won't parse URLs that don't end in .git
if not url_to_parse.endswith('.git'):
url_to_parse += '.git'
parsed = giturlparse.parse(url_to_parse)
url = parsed.resource
if not url.startswith('https://'):
url = 'https://' + url
owner = parsed.owner
repo = parsed.name
except giturlparse.parser.ParserError:
pass
if not args.dryrun and args.interface not in interfaces.INTERFACES:
print('Valid inline-plz config not found')
return 1
Expand All @@ -163,14 +144,7 @@ def inline(args):
print_messages(messages)
return 0
try:
my_interface = interfaces.INTERFACES[args.interface](
owner,
repo,
args.pull_request,
args.branch,
args.token,
url
)
my_interface = interfaces.INTERFACES[args.interface](args)
if my_interface.post_messages(messages, args.max_comments) and not args.zero_exit:
return 1
except KeyError:
Expand Down
4 changes: 2 additions & 2 deletions inlineplz/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def __init__(self):
self.messages = {}

def add_message(self, path, line, message):
path = os.path.relpath(path).replace('\\', '/').strip()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the path updating to the interface-specific code

path = path.replace('\\', '/').strip()
# replace backticks with single quotes to avoid markdown escaping issues
message = message.replace('`', '\'').strip()
try:
Expand All @@ -42,7 +42,7 @@ def get_messages(self):
class Message(object):

def __init__(self, path, line_number):
self.path = os.path.relpath(path).replace('\\', '/')
self.path = path.replace('\\', '/').strip()
self.line_number = int(line_number)
self.comments = set()

Expand Down
3 changes: 2 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
'uritemplate.py',
'dirtyjson',
'python-dateutil',
'git-url-parse'
'git-url-parse',
'requests'
]

test_requirements = [
Expand Down