First pass at Swarm interface integration for inline comments #193

wparks · 2018-03-16T17:21:34Z

No description provided.

wparks · 2018-03-16T17:22:54Z

inlineplz/message.py

@@ -15,7 +15,7 @@ def __init__(self):
        self.messages = {}

    def add_message(self, path, line, message):
-        path = os.path.relpath(path).replace('\\', '/').strip()


Moving the path updating to the interface-specific code

guykisel · 2018-03-16T17:32:43Z

inlineplz/interfaces/swarm.py

+        parameters = "topic={}&max={}".format(self.topic, max_comments)
+        url = "https://{}/api/{}/comments".format(self.host, self.version)
+        r = requests.get(url, auth=(self.username, self.password))
+        if (r.status_code != 200):


if r.status_code != requests.codes.ok:

guykisel · 2018-03-16T17:33:09Z

inlineplz/interfaces/swarm.py

+    def is_duplicate(comments, msg, body):
+        for comment in comments:
+            try:
+                if (comment["context"]["rightLine"] == msg.line_number):


the parens here are unnecessary

guykisel · 2018-03-16T17:33:22Z

inlineplz/interfaces/swarm.py

+            try:
+                if (comment["context"]["rightLine"] == msg.line_number):
+                    if (comment["context"]["file"] == msg.path):
+                        if (comment["body"].strip() == body.strip()):


you could just chain these into one if

guykisel · 2018-03-16T17:34:26Z

inlineplz/interfaces/swarm.py

+from inlineplz.interfaces.base import InterfaceBase
+
+class SwarmInterface(InterfaceBase):
+    def __init__(self, username, password, host, topic, version='v8'):


ok so this is where you wanted to change this to taking some sort of config object that would be created in main.py right?

updates from PR comments

and added comments to SwarmInterface

guykisel · 2018-03-16T21:30:42Z

inlineplz/interfaces/github.py

+                # 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)


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

(and delete the giturlparse import from main.py)

guykisel · 2018-03-16T21:31:51Z

inlineplz/interfaces/swarm.py

@@ -0,0 +1,98 @@
+# -*- coding: utf-8 -*-
+
+import requests


requests isnt a builtin module so it should be in a 2nd import group https://www.python.org/dev/peps/pep-0008/#imports

Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports.

guykisel · 2018-03-16T21:32:51Z

inlineplz/interfaces/swarm.py

+        self.password = args.password
+        self.host = args.host
+        self.topic = args.topic
+        self.version = 'v8'


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

guykisel · 2018-03-16T21:38:21Z

Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/bin/inline-plz", line 11, in <module>
    load_entry_point('inlineplz', 'console_scripts', 'inline-plz')()
  File "/home/travis/build/guykisel/inline-plz/inlineplz/main.py", line 61, in main
    result = inline(args)
  File "/home/travis/build/guykisel/inline-plz/inlineplz/main.py", line 148, in inline
    my_interface = interfaces.INTERFACES[args.interface](args)
  File "/home/travis/build/guykisel/inline-plz/inlineplz/interfaces/github.py", line 63, in __init__
    if branch and not pr:
NameError: name 'branch' is not defined

from the failed CI. i added args.branch earlier today

guykisel · 2018-03-16T21:43:31Z

inlineplz/main.py

+    parser.add_argument('--url', type=str, default=None)
+    parser.add_argument('--username', type=str)
+    parser.add_argument('--password', type=str)
+    parser.add_argument('--host', type=str)


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

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

guykisel · 2018-03-16T21:43:55Z

inlineplz/main.py

+    parser.add_argument('--username', type=str)
+    parser.add_argument('--password', type=str)
+    parser.add_argument('--host', type=str)
+    parser.add_argument('--topic', type=str)


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

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

guykisel · 2018-03-16T21:44:13Z

inlineplz/main.py

-    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)


do we need some sort of auth for p4?

and/or for swarm?

yes, it's in the username and password.

guykisel · 2018-03-16T21:45:22Z

inlineplz/main.py

-    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)


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

Sure, we can do that.

guykisel · 2018-03-19T17:09:22Z

inlineplz/interfaces/swarm.py

+        """
+        review_id = args.review_id
+        try:
+            review_id = int(review_id)


are swarm review IDs always integers?

guykisel · 2018-03-19T17:09:53Z

inlineplz/interfaces/swarm.py

+                continue
+            messages_to_post += 1
+            body = self.format_message(msg)
+            proc = subprocess.Popen(["p4", "fstat", "-T", "depotFile", msg.path], stdout=subprocess.PIPE)


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

guykisel · 2018-03-19T17:10:15Z

inlineplz/interfaces/swarm.py

+            proc = subprocess.Popen(["p4", "fstat", "-T", "depotFile", msg.path], stdout=subprocess.PIPE)
+            output = proc.stdout.read()
+            l = output.split()
+            if (len(l) != 3):


unnecessary parens

guykisel · 2018-03-19T17:14:16Z

the readme still references token:

You'll also need to provide the following either in the command line or via environment variables:

owner: the repo organization/owner
repo: the repo name
token: your auth token (encrypt this, don't put this in plaintext in any public configurations!)
url: the url of your scm host
interface: the type of scm host (such as github)

guykisel · 2018-03-19T17:21:25Z

looking at the console logs from the last run, although it passed, it doesnt look like it tried to run the github interface:

Parsing of stylint took 0 seconds
Running linter: jsonlint
b'+ [email protected]\nadded 124 packages in 5.157s\n\nnpm WARN deprecated [email protected]: Package no longer supported. Contact [email protected] for more info.\nnpm WARN saveError ENOENT: no such file or directory, open \'/home/travis/build/guykisel/inline-plz/package.json\'\nnpm WARN enoent ENOENT: no such file or directory, open \'/home/travis/build/guykisel/inline-plz/package.json\'\nnpm WARN inline-plz No description\nnpm WARN inline-plz No repository field.\nnpm WARN inline-plz No README data\nnpm WARN inline-plz No license field.\nnpm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/fsevents):\nnpm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})'
['node_modules/.bin/jsonlint', '-c', '-q']
Installation and running of jsonlint took 5 seconds
Parsing of jsonlint took 0 seconds
675 lint messages found
inline-plz version: 0.26.1
Python version: 3.6.3 (default, Nov 28 2017, 18:47:21) 
[GCC 4.8.4]
Last SHA: 1206f79c7a7ba0c9f5a70db6a857a9a164d6e61e
inline-plz version: 0.26.1
Python version: 3.6.3 (default, Nov 28 2017, 18:47:21) 
[GCC 4.8.4]
inline-plz ran for 40 seconds
inline-plz returned exit code 0

travis_time:end:03584e56:start=1521260769120624562,finish=1521260809879847019,duration=40759222457
�[0K
�[32;1mThe command "inline-plz --autorun" exited with 0.�[0m```

guykisel · 2018-03-19T17:21:37Z

so i think something might be wrong with the github integration

guykisel · 2018-03-19T17:50:39Z

inlineplz/interfaces/swarm.py

-            output = proc.stdout.read()
+            try:
+                output = subprocess.check_output(["p4", "fstat", "-T", "depotFile", msg.path])
+            except subprocess.CalledProcessError as procError:


proc_error would be the pythonic way to name a variable

guykisel · 2018-03-19T17:51:08Z

inlineplz/interfaces/swarm.py

@@ -70,8 +73,10 @@ def post_comment(self, body, path, line_number):
            'context[file]': path,
            'context[rightLine]': line_number
        }
-        print("".format(payload))
-        requests.post(url, auth=(self.username, self.password), data=payload)
+        #print("{}".format(payload))


delete this?

guykisel · 2018-03-19T17:51:19Z

inlineplz/interfaces/swarm.py

-        requests.post(url, auth=(self.username, self.password), data=payload)
+        #print("{}".format(payload))
+        response = requests.post(url, auth=(self.username, self.password), data=payload)
+        if (response.status_code != requests.codes.ok):


unnecessary parens

First pass at Swarm interface integration for inline comments

3fb4891

wparks commented Mar 16, 2018

View reviewed changes

guykisel reviewed Mar 16, 2018

View reviewed changes

riot-wparks added 4 commits March 16, 2018 11:09

Changed how args are passed to interfaces

5bb35a4

updates from PR comments

Fixes after linter tests

83bfc14

Merge branch 'master' into swarm_interface

4d264f0

and added comments to SwarmInterface

prints with parathensis for that python3

44c2585

guykisel reviewed Mar 16, 2018

View reviewed changes

fixes from failed CI and PR comments

136d13e

guykisel reviewed Mar 16, 2018

View reviewed changes

riot-wparks added 6 commits March 16, 2018 15:02

integrate some PR comments

67cf577

fix breakage

4c1a244

fix password break

0ab9b87

minor adjustments and test print

b58bbbe

python3

e0873a3

change pull_request to review_id

51e87e1

guykisel reviewed Mar 19, 2018

View reviewed changes

fixes based on comments

f67945d

guykisel reviewed Mar 19, 2018

View reviewed changes

guykisel mentioned this pull request Mar 27, 2018

Support P4 Swarm reviews #183

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at Swarm interface integration for inline comments #193

First pass at Swarm interface integration for inline comments #193

wparks commented Mar 16, 2018

wparks Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

guykisel commented Mar 16, 2018 •

edited

Loading

guykisel Mar 16, 2018

wparks Mar 16, 2018

guykisel Mar 16, 2018

wparks Mar 16, 2018

guykisel Mar 16, 2018

guykisel Mar 16, 2018

wparks Mar 16, 2018

guykisel Mar 16, 2018

wparks Mar 16, 2018

guykisel Mar 19, 2018

wparks Mar 19, 2018

guykisel Mar 19, 2018

guykisel Mar 19, 2018

guykisel commented Mar 19, 2018

guykisel commented Mar 19, 2018

guykisel commented Mar 19, 2018

guykisel Mar 19, 2018

guykisel Mar 19, 2018

guykisel Mar 19, 2018

First pass at Swarm interface integration for inline comments #193

Are you sure you want to change the base?

First pass at Swarm interface integration for inline comments #193

Conversation

wparks commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guykisel commented Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guykisel commented Mar 19, 2018

guykisel commented Mar 19, 2018

guykisel commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guykisel commented Mar 16, 2018 •

edited

Loading