[PATCH] D46192: Script to match open Phabricator reviews with potential reviewers

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

There's a few small inconsistencies (such as spaces after print, between operators, etc) that could be automatically solved by running a formatter (e.g. YAPF) over the source code.



================
Comment at: utils/Reviewing/find_interesting_reviews.py:3
+
+from phabricator import Phabricator
+import pickle
----------------
Nit: can we sort the imports alphabetically? 


================
Comment at: utils/Reviewing/find_interesting_reviews.py:84
+            pickle.dump(self.__dict__, f)
+        print ("wrote cache to disk, most_recent_info=%s"
+               % (datetime.fromtimestamp(self.most_recent_info)
----------------
Nit: You have different approaches to string formatting throughout this file. Personally I prefer the `"".format()` approach but as long as we're consistent I'd be happy. 


================
Comment at: utils/Reviewing/find_interesting_reviews.py:149
+
+        # merge adjacent and overlapping ranges
+        t = []
----------------
Nit: We usually make comments full sentences in LLVM. 


================
Comment at: utils/Reviewing/find_interesting_reviews.py:432
+    try:
+        # print(cmd)
+        output = subprocess.check_output(cmd, shell=True,
----------------
I'm guessing the commented out code is for logging? Maybe we could use the logger for this with a low verbosity? 


================
Comment at: utils/Reviewing/find_interesting_reviews.py:532
+    git_repos_directory = "git_repos"
+    git_repo_metadata = (("llvm", "https://llvm.org/git/llvm.git"),)
+    for name, url in git_repo_metadata:
----------------
Maybe turn this into a constant?


================
Comment at: utils/Reviewing/find_interesting_reviews.py:589
+
+main()
----------------
```
if __name__ == "__main__":
    main()
```


https://reviews.llvm.org/D46192