review-tools

Merge ~pfsmorigo/review-tools:issues_email into review-tools:master

  1. Git
  2. lp:~pfsmorigo/review-tools
  3. issues_email
  4. Merge into master
Proposed by Paulo Flabiano Smorigo
Status: Merged
Merged at revision: ceb2125fce5532f6ce857599eb1b095497ce1cf4
Proposed branch: ~pfsmorigo/review-tools:issues_email
Merge into: review-tools:master
Diff against target: 73 lines (+16/-12)
2 files modified
reviewtools/sr_lint.py (+7/-6)
reviewtools/tests/test_sr_lint.py (+9/-6)
Reviewer Review Type Date Requested Status
Alex Murray Approve
Emilia Torino Approve
Review via email: mp+426492@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Emilia Torino (emitorino) wrote :

LGTM, thanks!

review: Approve
Revision history for this message
Alex Murray (alexmurray) wrote :

LGTM but I wonder if using linkskey in ["xxx", "yyy"] might be easier to read than linkskey == "xxx" or linkskey = "yyy" etc (plus it is easier to just add another case in the future by adding it to the list as well)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
2index 15e31c9..2a9482a 100644
3--- a/reviewtools/sr_lint.py
4+++ b/reviewtools/sr_lint.py
5@@ -399,15 +399,16 @@ class SnapReviewLint(SnapReview):
6 return
7
8 if not (
9- linkskey == "contact"
10- and valid_email.match(link)
11+ (
12+ linkskey in ["contact", "issues"] and valid_email.match(link)
13+ )
14 or valid_url.match(link)
15 ):
16 t = "error"
17- s = (
18- "%s link must have one of http|https schemes or it must be an email address: %s"
19- % (linkskey, link)
20- )
21+ s = "%s link must have one of http|https schemes" % linkskey
22+ if linkskey in ["contact", "issues"]:
23+ s += " or it must be an email address"
24+ s += ": %s " % link
25 self._add_result(t, n, s, manual_review=True)
26 return
27
28diff --git a/reviewtools/tests/test_sr_lint.py b/reviewtools/tests/test_sr_lint.py
29index f32faa9..2f08d21 100644
30--- a/reviewtools/tests/test_sr_lint.py
31+++ b/reviewtools/tests/test_sr_lint.py
32@@ -985,7 +985,10 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
33 "another@email.com",
34 ],
35 "donation": ["http://donate.me"],
36- "issues": ["https://bugs.launchpad.net/snapcraft/+filebug"],
37+ "issues": [
38+ "https://bugs.launchpad.net/snapcraft/+filebug",
39+ "report@email.com",
40+ ],
41 }
42 self.set_test_snap_yaml("links", links)
43 c = SnapReviewLint(self.test_name)
44@@ -1005,7 +1008,7 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
45
46 def test_check_link_invalid_url_no_http_prefix(self):
47 """Test check_link_invalid_url_no_http_prefix"""
48- self.set_test_snap_yaml("links", {"website": "www.snapcraft.io"})
49+ self.set_test_snap_yaml("links", {"website": ["www.snapcraft.io"]})
50 c = SnapReviewLint(self.test_name)
51 c.check_links()
52 r = c.review_report
53@@ -1014,16 +1017,16 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
54
55 def test_check_link_invalid_email(self):
56 """Test check_link_invalid_email"""
57- self.set_test_snap_yaml("links", {"contact": "invalid(at)email.com"})
58+ self.set_test_snap_yaml("links", {"contact": ["invalid(at)email.com"]})
59 c = SnapReviewLint(self.test_name)
60 c.check_links()
61 r = c.review_report
62 expected_counts = {"info": None, "warn": 0, "error": 1}
63 self.check_results(r, expected_counts)
64
65- def test_check_link_email_not_in_contacts(self):
66- """Test check_link_email_not_in_contacts"""
67- self.set_test_snap_yaml("links", {"issues": "valid@email.com"})
68+ def test_check_link_email_not_in_contacts_or_issues(self):
69+ """Test check_link_email_not_in_contacts_or_issues"""
70+ self.set_test_snap_yaml("links", {"donation": ["valid@email.com"]})
71 c = SnapReviewLint(self.test_name)
72 c.check_links()
73 r = c.review_report

Subscribers

People subscribed via source and target branches