-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
3fb4891
5bb35a4
83bfc14
4d264f0
44c2585
136d13e
67cf577
4c1a244
0ab9b87
b58bbbe
e0873a3
51e87e1
f67945d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
# -*- coding: utf-8 -*- | ||
|
||
import requests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. requests isnt a builtin module so it should be in a 2nd import group https://www.python.org/dev/peps/pep-0008/#imports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
import random | ||
import subprocess | ||
|
||
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 | ||
self.version = 'v8' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would prefer if we used |
||
output = proc.stdout.read() | ||
l = output.split() | ||
if (len(l) != 3): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
r = 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) as exceptError: | ||
continue | ||
return False | ||
|
||
@staticmethod | ||
def format_message(message): | ||
if not message.comments: | ||
return '' | ||
return ( | ||
'```\n' + | ||
'\n'.join(sorted(list(message.comments))) + | ||
'\n```' | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,11 @@ def main(): | |
parser.add_argument('--branch', type=str) | ||
parser.add_argument('--token', type=str) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we somehow combine password / token into a single auth arg? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, we can do that. |
||
parser.add_argument('--host', type=str) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we rename this to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
@@ -120,28 +124,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 | ||
|
@@ -163,14 +145,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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ def __init__(self): | |
self.messages = {} | ||
|
||
def add_message(self, path, line, message): | ||
path = os.path.relpath(path).replace('\\', '/').strip() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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() | ||
|
||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)