fedora-qa / blockerbugs

Clone
Source Code
GIT

#161 Revise Pagure comment parsers, make more liberal (#153) (#155)
Merged 4 years ago by kparal. Opened 4 years ago by adamwill.
fedora-qa/ adamwill/blockerbugs pagure-parser-rewrite  into  develop

file modified
+65 -55 
@@ -16,45 +16,59 @@ 
 

      'previousrelease'
 

  ]
 

  

- 

- VOTE_KEYWORDS = [' '.join(v) for v in
 

-                  itertools.product(TRACKER_KEYWORDS, ['-1', '0', '+1'])]
 

- 

- OUTCOME_KEYWORDS = [''.join(r) for r in
 

-                    itertools.product(['accepted', 'rejected'], TRACKER_KEYWORDS)]
 

- 

- REVOTE_KEYWORDS = ['revote ' + t for t in TRACKER_KEYWORDS]
 

- 

- #ADMIN_KEYWORDS = RESULT_KEYWORDS + REVOTE_KEYWORDS
 

+ VOTES = ("+1", "0", "-1")
 

+ TRACKER_RE = r'((beta|final)(blocker|f(?:reeze)?e(?:xception)?)|0day|previousrelease)'
 

+ OUTCOME_RE = r'(accepted|rejected)'
 

  

  SUMMARY_HEADER = ""
 

  CLOSED_VOTES_HEADER = "The following votes have been closed:"
 

  

- 

- def agreed_parser(line):
 

+ def agreed_revote_parser(line, keyword, matcher, summary=True):
 

+     """parsing 'agreed' and 'revote' is done similarly, but with
 

+     different keywords and REs, and 'agreed' allows extra text after
 

+     the matches (a summary) but 'revote' does not.
 

+     """
 

      out = []
 

-     outcome_pattern = '|'.join(OUTCOME_KEYWORDS)
 

-     #pattern = '^agreed (({}) )+.*$'.format(outcome_pattern)
 

-     pattern = '^agreed ({})( ({}))*( .*)?$'.format(outcome_pattern, outcome_pattern)
 

-     m = re.match(pattern, line)
 

-     if m:
 

-         for outcome in OUTCOME_KEYWORDS:
 

-             if outcome in line:
 

-                 out.append('agreed ' + outcome)
 

+     # parsing word by word avoids complex and fragile regex syntax
 

+     # first word is the keyword, so we skip it
 

+     words = line.split()[1:]
 

+     for word in words:
 

+         if matcher.fullmatch(word):
 

+             # if we get a match, take it into out for now
 

+             out.append(keyword + " " + word)
 

+         else:
 

+             # if not, we break (and then return the matches we got so
 

+             # far) if summary is allowed, return empty if not
 

+             if summary:
 

+                 break
 

+             return []
 

      return out
 

  

- def revote_parser(line):
 

+ def vote_parser(line):
 

+     """Look for votes in lines. We accept pairs of a vote and match of
 

+     TRACKER_RE, in either order, and nothing else; any other text
 

+     invalidates the line.
 

+     """
 

      out = []
 

-     trackers = '|'.join(TRACKER_KEYWORDS)
 

-     pattern = '^revote (({}) ?)+$'.format(trackers)
 

-     m = re.match(pattern, line)
 

-     if m:
 

-         for tracker in TRACKER_KEYWORDS:
 

-             if tracker in line:
 

-                 out.append('revote ' + tracker)
 

+     words = line.split()
 

+     # if we have an odd number of words we don't have a clean vote line
 

+     if (len(words) % 2) != 0:
 

+         return out
 

+     trackmatcher = re.compile(TRACKER_RE)
 

+     # split list into pairs:
 

+     # https://stackoverflow.com/questions/312443
 

+     for pair in (words[i:i + 2] for i in range(0, len(words), 2)):
 

+         if trackmatcher.fullmatch(pair[0]) and pair[1] in VOTES:
 

+             out.append(" ".join(pair))
 

+         elif pair[0] in VOTES and trackmatcher.fullmatch(pair[1]):
 

+             pair.reverse()
 

+             out.append(" ".join(pair))
 

+         else:
 

+             # we found something other than a vote pair, so per
 

+             # Kamil's Strict Vote Parsing Regime, we reject the line
 

+             return []
 

      return out
 

  

- 

  def bot_loop_detected(comments):
 

      is_bot_comment = [comment.user == app.config['PAGURE_BOT_USERNAME'] for comment in comments]
 

      if len(is_bot_comment) < app.config['PAGURE_BOT_LOOP_THRESHOLD']:
 
@@ -80,20 +94,27 @@ 
 

      def keywords(self):
 

          out = []
 

          for line in self.text.lower().split('\n'):
 

-             line = line.rstrip()
 

-             line = re.sub(' +', ' ', line)
 

-             if line in VOTE_KEYWORDS:
 

-                 out.append(line)
 

-             elif line.startswith('agreed'):
 

-                 out.extend(agreed_parser(line))
 

-             elif line.startswith('revote'):
 

-                 out.extend(revote_parser(line))
 

+             line = line.strip()
 

+             out.extend(vote_parser(line))
 

+             if line.startswith('agreed '):
 

+                 keyword = "agreed"
 

+                 matcher = re.compile(OUTCOME_RE + TRACKER_RE)
 

+                 summary = True
 

+                 out.extend(agreed_revote_parser(line, keyword, matcher, summary))
 

+             elif line.startswith('revote '):
 

+                 keyword = "revote"
 

+                 matcher = re.compile(TRACKER_RE)
 

+                 summary = False
 

+                 out.extend(agreed_revote_parser(line, keyword, matcher, summary))
 

+         # most strategic place to treat 'freezeexception' as 'fe'
 

+         out = [keyword.replace("freezeexception", "fe") for keyword in out]
 

          return out
 

  

      def is_summary_post_of(self, tracker_keyword):
 

          is_bot = self.user == app.config['PAGURE_BOT_USERNAME']
 

          is_summary = CLOSED_VOTES_HEADER.lower() in self.text.lower()
 

-         is_tracker_relevant = tracker_keyword in self.text.lower()
 

+         is_tracker_relevant = (tracker_keyword in self.text.lower() or
 

+                                tracker_keyword.replace("fe", "freezeexception") in self.text.lower())

I'm somewhat concerned about this. It only works because we don't have the "fe" substring anywhere else in our keywords. But if we did, or if we change it in the future, this will start causing troubles. I'd rather make this change more systematic and safer. I'll add a patch.

 
  

          return is_bot and is_summary and is_tracker_relevant
 

  
@@ -113,18 +134,11 @@ 
 

          # }
 

          self.votes = {}
 

  

-     def _filter_relevant(self, keywords):
 

-         accepted_votes = [self.accepted_keyword + v for v in [' +1', ' 0', ' -1']]
 

-         accepted_agreed = ['agreed ' + o + self.accepted_keyword for o in ['accepted', 'rejected']]
 

-         accepted_revote = ['revote ' + self.accepted_keyword]
 

-         accepted_keywords = accepted_votes + accepted_agreed + accepted_revote
 

-         return [keyword for keyword in keywords if keyword in accepted_keywords]
 

- 

      def parse_comment(self, comment):
 

          if self.need_summary_post:
 

              self.need_summary_post = not comment.is_summary_post_of(self.accepted_keyword)
 

  

-         keywords = self._filter_relevant(comment.keywords())
 

+         keywords = [keyword for keyword in comment.keywords() if self.accepted_keyword in keyword]
 

          for keyword in keywords:
 

              # FIXME log rejected (non admin) users?
 

              if keyword.startswith('agreed') and comment.user_is_admin():
 
@@ -137,7 +151,7 @@ 
 

              else:
 

                  if not self.open:
 

                      continue
 

-                 for vote in ['-1', '0', '+1']:
 

+                 for vote in VOTES:
 

                      if keyword.endswith(vote):
 

                          self.votes[comment.user] = {
 

                              'vote': vote,
 
@@ -146,11 +160,7 @@ 
 

          return keywords
 

  

      def enumerate_votes(self):
 

-         out = {
 

-             '-1': [],
 

-             '0': [],
 

-             '+1': []
 

-         }
 

+         out = {vote: [] for vote in VOTES}
 

          for user, vote in self.votes.items():
 

              out[vote['vote']].append((user, vote['comment_id']))
 

  
@@ -163,8 +173,8 @@ 
 

      nicer = {
 

          'betablocker': 'BetaBlocker',
 

          'finalblocker': 'FinalBlocker',
 

-         'betafe': 'BetaFE',
 

-         'finalfe': 'FinalFE',
 

+         'betafe': 'BetaFreezeException',
 

+         'finalfe': 'FinalFreezeException',
 

          '0day': '0Day',
 

          'previousrelease': 'PreviousRelease',
 

          'accepted': 'Accepted',
 
@@ -209,7 +219,7 @@ 
 

  

          md_text += tracker_summary(tracker_name, tracker)
 

  

-         for neco in ['+1', '0', '-1']:
 

+         for neco in VOTES:
 

              votes[neco] = [link(user, comment_id) for (user, comment_id) in votes[neco]]
 

              if len(votes[neco]) == 0:
 

                  continue

file modified
+281 -93 
@@ -6,6 +6,7 @@ 
 

  from blockerbugs.util import pagure_interface
 

  from blockerbugs.util import pagure_bot
 

  

+ EXTENDED_TRACKERS = ["betafreezeexception", "finalfreezeexception"] + pagure_bot.TRACKER_KEYWORDS
 

  

  def powerset(iterable):
 

      s = list(iterable)
 
@@ -38,13 +39,14 @@ 
 

  

  

  class TestPagureBotBasics(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

-     def test_basic_vote(self, vote, keyword):
 

+     def test_basic_vote(self, vote, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s" % (keyword, vote),
 

+                 'comment': "%s %s" % ((vote, keyword) if inversed else (keyword, vote)),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -52,14 +54,14 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote,
 

                  'comment_id': 1
 

              }
 

          }
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      def test_basic_vote_repeated(self, keyword):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 
@@ -79,7 +81,7 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '-1',
 

                  'comment_id': 2
 
@@ -88,7 +90,7 @@ 
 

  

  

  class TestPagureBotTypos(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

      def test_typo(self, sign, keyword):
 

          keyword_typo = list(keyword)
 
@@ -105,17 +107,18 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

  

  class TestPagureBotGotchas(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['＋', '–'])  # these chars are not + or -
 

-     def test_unicode_plus(self, sign, keyword):
 

+     def test_unicode_plus(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s1" % (keyword, sign),
 

+                 'comment': "%s1 %s" % (sign, keyword) if inversed else "%s %s1" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -123,15 +126,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_eleven(self, sign, keyword):
 

+     def test_plusminus_eleven(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s11" % (keyword, sign),
 

+                 'comment': "%s11 %s" % (sign, keyword) if inversed else "%s %s11" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -139,15 +143,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_two(self, sign, keyword):
 

+     def test_plusminus_two(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s2" % (keyword, sign),
 

+                 'comment': "%s2 %s" % (sign, keyword) if inversed else "%s %s2" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -155,15 +160,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_typo(self, sign, keyword):
 

+     def test_plusminus_typo(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s!" % (keyword, sign),
 

+                 'comment': "%s! %s" % (sign, keyword) if inversed else "%s %s!" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -171,15 +177,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_float(self, sign, keyword):
 

+     def test_plusminus_float(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s0.5" % (keyword, sign),
 

+                 'comment': "%s0.5 %s" % (sign, keyword) if inversed else "%s %s0.5" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -187,15 +194,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_emoji(self, sign, keyword):
 

+     def test_plusminus_emoji(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s👎" % (keyword, sign),
 

+                 'comment': "%s👎 %s" % (sign, keyword) if inversed else "%s %s👎" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -203,15 +211,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_plusminus_backtick(self, sign, keyword):
 

+     def test_plusminus_backtick(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "`%s %s1`" % (keyword, sign),
 

+                 'comment': "`%s1 %s`" % (sign, keyword) if inversed else "`%s %s1`" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -219,17 +228,18 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

  

  

  class TestPagureBotMultiline(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("sign", ['+', '-'])
 

-     def test_multiline_singlevote(self, sign, keyword):
 

+     def test_multiline_singlevote(self, sign, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "foobar\n%s %s1\nbarbaz" % (keyword, sign),
 

+                 'comment': "foobar\n%s1 %s\nbarbaz" % (sign, keyword) if inversed else "foobar\n%s %s1\nbarbaz" % (keyword, sign),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -237,24 +247,26 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s1' % sign,
 

                  'comment_id': 1
 

              }
 

          }
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),
 

                                        ('-1', '0'), ('0', '-1'), ('0', '+1')])
 

-     def test_multiline_multivote_same(self, vote, keyword):
 

+     def test_multiline_multivote_same(self, vote, keyword, inversed):
 

          vote_first = vote[0]
 

          vote_second = vote[1]
 

+         first = (vote_first, keyword) if inversed[0] else (keyword, vote_first)
 

+         second = (vote_second, keyword) if inversed[1] else (keyword, vote_second)
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (keyword, vote_first,
 

-                                                              keyword, vote_second),
 

+                 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (first + second),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -262,27 +274,33 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote_second,
 

                  'comment_id': 1
 

              }
 

          }
 

  

-     @pytest.mark.parametrize("keyword", itertools.permutations(pagure_bot.TRACKER_KEYWORDS, 2))
 

+     @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))
 

+     @pytest.mark.parametrize("keyword", itertools.permutations(EXTENDED_TRACKERS, 2))
 

      @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),
 

                                        ('-1', '0'), ('0', '-1'), ('0', '+1'),
 

                                        ('0', '0'), ('-1', '-1'), ('+1', '+1')])
 

-     def test_multiline_multivote_different(self, vote, keyword):
 

+     def test_multiline_multivote_different(self, vote, keyword, inversed):
 

          keyword_first = keyword[0]
 

          keyword_second = keyword[1]
 

+         # if we hit two keywords which are synonyms for each other,
 

+         # move on, test is invalid
 

+         if keyword_first.replace("fe", "freezeexception") == keyword_second.replace("fe", "freezeexception"):
 

+             return None
 

          vote_first = vote[0]
 

          vote_second = vote[1]
 

+         first = (vote_first, keyword_first) if inversed[0] else (keyword_first, vote_first)
 

+         second = (vote_second, keyword_second) if inversed[1] else (keyword_second, vote_second)
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (keyword_first, vote_first,
 

-                                                              keyword_second, vote_second),
 

+                 'comment': "foobar\n%s %s\nbarbaz\n%s %s" % (first + second),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -290,13 +308,13 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword_first].votes == {
 

+         assert trackers[keyword_first.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote_first,
 

                  'comment_id': 1
 

              }
 

          }
 

-         assert trackers[keyword_second].votes == {
 

+         assert trackers[keyword_second.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote_second,
 

                  'comment_id': 1
 
@@ -304,8 +322,104 @@ 
 

          }
 

  

  

+ class TestPagureBotSinglelineMultivote(object):
 

+     @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

+     @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),
 

+                                       ('-1', '0'), ('0', '-1'), ('0', '+1')])
 

+     def test_singleline_multivote_same(self, vote, keyword, inversed):
 

+         vote_first = vote[0]
 

+         vote_second = vote[1]
 

+         first = (vote_first, keyword) if inversed[0] else (keyword, vote_first)
 

+         second = (vote_second, keyword) if inversed[1] else (keyword, vote_second)
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 # Votes should be accepted with second taking precedence
 

+                 'comment': "%s %s %s %s" % (first + second),
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

+             stub_admin_user1: {
 

+                 'vote': '%s' % vote_second,
 

+                 'comment_id': 1
 

+             }
 

+         }
 

+ 

+     @pytest.mark.parametrize("inversed", itertools.product([False, True], [False, True]))
 

+     @pytest.mark.parametrize("keyword", itertools.permutations(EXTENDED_TRACKERS, 2))
 

+     @pytest.mark.parametrize("vote", [('+1', '-1'), ('-1', '+1'), ('+1', '0'),
 

+                                       ('-1', '0'), ('0', '-1'), ('0', '+1'),
 

+                                       ('0', '0'), ('-1', '-1'), ('+1', '+1')])
 

+     def test_singleline_multivote_different(self, vote, keyword, inversed):
 

+         keyword_first = keyword[0]
 

+         keyword_second = keyword[1]
 

+         # if we hit two keywords which are synonyms for each other,
 

+         # move on, test is invalid
 

+         if keyword_first.replace("fe", "freezeexception") == keyword_second.replace("fe", "freezeexception"):
 

+             return None
 

+         vote_first = vote[0]
 

+         vote_second = vote[1]
 

+         first = (vote_first, keyword_first) if inversed[0] else (keyword_first, vote_first)
 

+         second = (vote_second, keyword_second) if inversed[1] else (keyword_second, vote_second)
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 # Both votes should be accepted
 

+                 'comment': "%s %s %s %s" % (first + second),
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers[keyword_first.replace("freezeexception", "fe")].votes == {
 

+             stub_admin_user1: {
 

+                 'vote': '%s' % vote_first,
 

+                 'comment_id': 1
 

+             }
 

+         }
 

+         assert trackers[keyword_second.replace("freezeexception", "fe")].votes == {
 

+             stub_admin_user1: {
 

+                 'vote': '%s' % vote_second,
 

+                 'comment_id': 1
 

+             }
 

+         }
 

+ 

+     def test_singleline_multivote_summary_rejected(self):
 

+         """Votes should be rejected due to additional text. We need
 

+         both comments to check we reject both even and odd word counts
 

+         """
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 'comment': "+1 FinalBlocker -1 BetaBlocker lalala",
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             },
 

+             {
 

+                 'id': 2,
 

+                 'comment': "+1 FinalBlocker -1 BetaBlocker lalala foofoofoo",
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers["finalblocker"].votes == {}
 

+         assert trackers["betablocker"].votes == {}
 

+ 

+ 

  class TestPagureBotAgreedRevote(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_needs_summary(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -326,15 +440,15 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '+1',
 

                  'comment_id': 1
 

              }
 

          }
 

-         assert trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_no_justification_needs_summary(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -355,15 +469,15 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '+1',
 

                  'comment_id': 1
 

              }
 

          }
 

-         assert trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_by_impostor_need_summary(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -391,15 +505,15 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '+1',
 

                  'comment_id': 1
 

              }
 

          }
 

-         assert trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))
 

+     @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))
 

      def test_agreed_multiple_accepts_need_summary(self, keywords, setup_stubs):
 

          outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -414,9 +528,9 @@ 
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

          for keyword in keywords:
 

-             assert trackers[keyword].need_summary_post
 

+             assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))
 

+     @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))
 

      def test_agreed_multiple_accepts_no_justification_need_summary(self, keywords, setup_stubs):
 

          outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -431,9 +545,9 @@ 
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

          for keyword in keywords:
 

-             assert trackers[keyword].need_summary_post
 

+             assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))
 

+     @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))
 

      def test_agreed_multiple_accepts_has_summary(self, keywords, setup_stubs):
 

          outcomes = " ".join(["accepted%s" % keyword for keyword in keywords])
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -455,9 +569,9 @@ 
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

          for keyword in keywords:
 

-             assert not trackers[keyword].need_summary_post
 

+             assert not trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      def test_revote_single(self, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 
@@ -491,12 +605,16 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

-         assert not trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

+         assert not trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

      @pytest.mark.parametrize("keyword_a, keyword_b",
 

-                              itertools.permutations(pagure_bot.TRACKER_KEYWORDS, 2))
 

+                              itertools.permutations(EXTENDED_TRACKERS, 2))
 

      def test_agreed_stops_counting_counts_other(self, keyword_a, keyword_b, setup_stubs):
 

+         # if we hit two keywords which are synonyms for each other,
 

+         # move on
 

+         if keyword_a.replace("fe", "freezeexception") == keyword_b.replace("fe", "freezeexception"):
 

+             return None
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 
@@ -522,13 +640,13 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword_a].votes == {
 

+         assert trackers[keyword_a.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '+1',
 

                  'comment_id': 1
 

              }
 

          }
 

-         assert trackers[keyword_b].votes == {
 

+         assert trackers[keyword_b.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '+1',
 

                  'comment_id': 1
 
@@ -541,13 +659,36 @@ 
 

  

  

  class TestPagureBotWhitespaces(object):
 

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

+     @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

+     def test_trailing_whitespace(self, vote, keyword, inversed):
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 'comment': "%s %s " % ((vote, keyword) if inversed else (keyword, vote)),
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

+             stub_admin_user1: {
 

+                 'vote': '%s' % vote,
 

+                 'comment_id': 1
 

+             }
 

+         }
 

+ 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

-     def test_trailing_whitespace(self, vote, keyword):
 

+     def test_leading_whitespace(self, vote, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s %s " % (keyword, vote),
 

+                 'comment': "   %s %s" % ((vote, keyword) if inversed else (keyword, vote)),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -555,20 +696,21 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote,
 

                  'comment_id': 1
 

              }
 

          }
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

-     def test_whitespaces_inside_vote(self, vote, keyword):
 

+     def test_whitespaces_inside_vote(self, vote, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s    %s" % (keyword, vote),
 

+                 'comment': "%s    %s" % ((vote, keyword) if inversed else (keyword, vote)),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -576,20 +718,66 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

              stub_admin_user1: {
 

                  'vote': '%s' % vote,
 

                  'comment_id': 1
 

              }
 

          }
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

-     def test_newline_inside_vote_ignored(self, vote, keyword):
 

+     def test_trailing_leading_inside_whitespace(self, vote, keyword, inversed):
 

          comments = [pagure_bot.Comment(c) for c in [
 

              {
 

                  'id': 1,
 

-                 'comment': "%s\n%s" % (keyword, vote),
 

+                 'comment': "   %s   %s  " % ((vote, keyword) if inversed else (keyword, vote)),
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {
 

+             stub_admin_user1: {
 

+                 'vote': '%s' % vote,
 

+                 'comment_id': 1
 

+             }
 

+         }
 

+ 

+     @pytest.mark.parametrize("inversed", [False, True])
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

+     @pytest.mark.parametrize("vote", ['+1', '-1', '0'])
 

+     def test_newline_inside_vote_ignored(self, vote, keyword, inversed):
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 'comment': "%s\n%s" % ((vote, keyword) if inversed else (keyword, vote)),
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             }
 

+         ]]
 

+ 

+         trackers = pagure_bot.parse_comments_to_trackers(comments)
 

+         assert trackers[keyword.replace("freezeexception", "fe")].votes == {}
 

+ 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

+     @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

+     def test_agreed_single_acceptedrejected_leading_wspace_needs_summary(self, outcome, keyword, setup_stubs):
 

+         comments = [pagure_bot.Comment(c) for c in [
 

+             {
 

+                 'id': 1,
 

+                 'comment': "%s +1" % keyword,
 

+                 'user': {
 

+                     'name': stub_admin_user1
 

+                 }
 

+             },
 

+             {
 

+                 'id': 2,
 

+                 'comment': "   AGREED %s%s foo bar" % (outcome, keyword),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -597,9 +785,9 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].votes == {}
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_trailing_wspace_needs_summary(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -620,9 +808,9 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_multi_wspace_inside_needs_summary(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -635,7 +823,7 @@ 
 

              },
 

              {
 

                  'id': 2,
 

-                 'comment': "AGREED     %s%s foo bar   " % (outcome, keyword),
 

+                 'comment': "   AGREED     %s%s   foo bar   " % (outcome, keyword),
 

                  'user': {
 

                      'name': stub_admin_user1
 

                  }
 
@@ -643,9 +831,9 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert trackers[keyword].need_summary_post
 

+         assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keyword", pagure_bot.TRACKER_KEYWORDS)
 

+     @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS)
 

      @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected'])
 

      def test_agreed_single_acceptedrejected_newline_inside(self, outcome, keyword, setup_stubs):
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -666,9 +854,9 @@ 
 

          ]]
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

-         assert not trackers[keyword].need_summary_post
 

+         assert not trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

-     @pytest.mark.parametrize("keywords", powerset(pagure_bot.TRACKER_KEYWORDS))
 

+     @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS))
 

      def test_agreed_multiple_accepts_multi_wspace_inside_need_summary(self, keywords, setup_stubs):
 

          outcomes = "   ".join(["accepted%s" % keyword for keyword in keywords])
 

          comments = [pagure_bot.Comment(c) for c in [
 
@@ -683,7 +871,7 @@ 
 

  

          trackers = pagure_bot.parse_comments_to_trackers(comments)
 

          for keyword in keywords:
 

-             assert trackers[keyword].need_summary_post
 

+             assert trackers[keyword.replace("freezeexception", "fe")].need_summary_post
 

  

  

  class TestPagureBotHaywire(object):

This is an alternative to #160 that rewrites the line parsers.
Broadly we use a small set of simple regexes as constants (rather
than constructing more complex regexes on the fly from keyword
constants). We factor out the similarity in how the former
agreed_parser and revote_parser worked. We also fix #155 by
accepting both "FE" and "FreezeException".

We fix #153 even harder than required, by accepting any number of
pairs of TRACKER and VOTE in any order with arbitrary whitespace
at the start of a line (so you can vote multiple times on a line
if you like, as on IRC). For agreeds, we similarly now accept many
matches at the start of a line, but as soon as we see non-match
text, we stop parsing but keep the matches so far. This avoids
doing anything with keywords that appear in the summary, as that
might cause an unexpected result. For revotes we only accept the
line if it only consists of the keyword followed by tracker
identifiers; this is the same as before, but using the same word-
by-word parsing approach as the others.

The on-the-fly regexes that were being used here behaved a bit...
oddly...in general, if you extract them out and play around with
them. The old code avoided most consequences of the weirdness by
only using them for initial matching and then not using capture
groups but just iterating over the KEYWORDS constants, but it
still made things a bit hard to follow. I think it's clearer this
way. This way also deals elegantly with whitespace issues.

I stole the inverted tests from Lukas' #160, thanks.

Pagure: ditch _filter_relevant for a simpler check

We don't need to be so explicit here, we do enough "filtering"
of these keywords elsewhere. Just a simple match of the specific
tracker this instance is for being in the text is enough.

rebased onto b9bf794
4 years ago

Build succeeded.

rebased onto b60b0d2
4 years ago

Build succeeded.

rebased onto e27a3fc
4 years ago

Build succeeded.

rebased onto fc56df6
4 years ago

Build succeeded.

Sure! This looks better :thumbsup:

I was a bit unsure about the _filter_relevant removal which is why I made it a separate commit. It does really seem like it's unnecessary, though, and we pass all tests without it.

yes, _filter_relevant seems to be redundant and I have no problem believing the test suite as it is quite comprehensive.

LGTM

Metadata Update from @kparal:
- Request assigned
4 years ago

Why make this RE so complicated? Shouldn't this have the same effect with better readability?

((beta|final)(blocker|fe|freezeexception)|0day|previousrelease)

Honestly I'm not very fond of the keyword swapping changes together with allowing multiple votes on a single line together with allowing arbitrary text afterwards. There was a reason why I chose a fairly strict voting language - it is easy to read (and therefore review and spot errors) and it's hard to vote by accident. I also find it easy to remember, because there's only one way to write it, instead of allowing many ways (and then you need to think - is this allowed or not?).

Let's go one by one.

Keyword swapping:

BetaBlocker +1
+1 BetaBlocker

I'd be OK with this change on its own, it's still very readable. I don't see much benefit, but it's OK.

Multiple votes on a line:

BetaBlocker +1 FinalBlocker -1

I'm not very fond of this, but if people scream to have this, then I'm not going to oppose it. My objections are that there is very little benefit - hitting an enter or a space bar doesn't really change anything. At the same time it lowers readability and allows you to make mistakes you won't notice. Compare:

BetaBlocker -1 FinalBloker -1 BetaFE +1

BetaBlocker -1
FinalBloker -1
BetaFE +1

It's easier to spot the missing letter when each vote is on a single line. Similarly:

BetaBlocker -1 BetaFE +1, FinalBocker -1

People might not notice the comma they made there by accident (my hands often type automatically, it's like they have a mind of their own). With your implementation, only the first two votes will be counted, not the last one, and they'll like not notice that, even if they check the updated summary - because most people don't have this attention to detail.

So, it's OK, but I'm not a fan and would rather avoid it.

If you combine both these approaches, it's an abomination:

BetaBlocker -1 +1 FinalBlocker +1 BetaFE

Do we really want to support this? I don't. This is a path to mistake votes and I don't understand how this can be perceived as better than the current system, honestly. With the current implementation, consider this:

BetaBlocker -1 +1 FinalBlocker -1 BetaFE -1

This is a mistake where the person typed BetaBlocker -1 and wanted to correct it to +1, but forgot/failed to press the backspace correctly and didn't really check the result afterwards. It parses well... just differently from what the person had intended.

I think we can support keyword swapping OR multiple votes on a single line, but we shouldn't support both.

Arbitrary text afterwards:

BetaBlocker +1 for sure

seems like a nice way to allow human-style language when voting. However, that system also allows this:

kparal:
BetaBlocker +1

frantisekz:
BetaBlocker +1? Are you crazy? This is a minor issue!

The more you relax the restrictions and allow natural expressions, the harder it will be for people to be aware of the rules at all times. Sure, this is an artificial example. But are we trading the right values here? The current voting system is clear and exact. The "natural" voting system will have its quirks which will not be obvious on a first glance. People will expect all of this to work:

BetaBlocker +1 for sure
BetaBlocker +1, definitely
BetaBlocker +1!!!

And guess what, some of them don't. You can definitely make the code more complicated to accommodate for this, but then you also need to consider whether you want to allow

BetaBlocker +100

which will surely someone use just for fun, if they can (but +99 will not work, more fun).

Allowing arbitrary text afterwards will also allow you to bury the vote somewhere inside a wall of text, so people won't be able to easily spot it:

This is all just nonsense. This issue is not a big problem
at all. It affects just certain hardware and people named
'Tom'. If we release the final version with it, it will be OK.
FinalBlocker -1 is my definite vote and I don't want to
hear about it anymore. Let's just close this discussion
once and for all and be done with it.

Similarly, it might cause people to vote without realizing:

FinalBlocker +1 was also my opinion two days ago, but since then,
we've discovered the root cause and we should definitely not block on it now.

I'd argue against allowing arbitrary text afterwards. Again, if people scream for it, shrug. But we should definitely not combine it with the other two changes above, because all together it would be a voting minefield.

Thoughts?

Keyword swapping:
BetaBlocker +1 +1 BetaBlocker
I'd be OK with this change on its own, it's still very readable. I don't see much benefit, but it's OK.

On IRC meetings, people often vote +1 FE or +1 Blocker, so it is more in line with that.

Multiple votes on a line:
BetaBlocker +1 FinalBlocker -1
I'm not very fond of this, but if people scream to have this, then I'm not going to oppose it. My objections are that there is very little benefit - hitting an enter or a space bar doesn't really change anything. At the same time it lowers readability and allows you to make mistakes you won't notice. Compare:

I'd say let's permit multiple votes per line for this cycle and see how people like it, shouldn't be big hassle to be more strict again.

BetaBlocker -1 BetaFE +1, FinalBocker -1
BetaBlocker -1 +1 FinalBlocker -1 BetaFE -1

Good catch, these lines should be rejected. If we go with multiple votes per line then either all keyword+vote pairs are successfully parsed or whole line is rejected (in the first line +1, is incorrect vote and in the second line -1 at the end is trailing arbitrary text).

Arbitrary text afterwards:
I'd argue against allowing arbitrary text afterwards. Again, if people scream for it, shrug. But we should definitely not combine it with the other two changes above, because all together it would be a voting minefield.

Huh, I thought that I wrote test that checks rejection of votes with trailing text, apparently not :innocent:

I'm against allowing arbitrary text afterwards, what you describe are exactly the reasons why I didn't implement it originally.

On IRC meetings, people often vote +1 FE or +1 Blocker, so it is more in line with that.

I know, and we can swap the order if people feel it's more comfortable for them. But if we allow multiple votes on a line, there should be just one way to type it, imo.

I'd say let's permit multiple votes per line for this cycle and see how people like it, shouldn't be big hassle to be more strict again.

The more you change stuff, the more they'll grumble. I'd rather wait until we're convinced that it's a good move, than just testing it on people. And again, I'm not completely against it. It just should be carefully combined with the other changes, if we want to do this.

Huh, I thought that I wrote test that checks rejection of votes with trailing text, apparently not 😇

I honestly didn't check the unit tests at all. I just saw that Adam implemented it and even explicitly mentioned it in a docstring (and so I assume he changed the unit test if there was some). So it's quite possible we have a test against trailing text currently (and if we don't have it, we should add it, assuming we agree on disallowing trailing text).

Why make this RE so complicated? Shouldn't this have the same effect with better readability?
((beta|final)(blocker|fe|freezeexception)|0day|previousrelease)

Oh, yeah, it should. I was trying to do something clever by always matching as "fe" even when the text was "freezeexception", but it doesn't work because marking a group as non-capturing only applies to the group itself, not a parent group (so the parent group still captures all the text). So yeah, it's not achieving anything and we could simplify it.

I'll check the details of unit tests and stuff. My big picture take on this is:

"As long as we're forcing people to vote by typing in free text without good guard rails, we should be permissive".

I don't like being strict on an operation that is relatively hard to "get right" and where we don't clearly communicate when you "got it wrong". Especially with the complicating factor that we've had a moderately intelligent system called adamw parsing votes in IRC for years, and the adamw parser certainly accepts sloppiness like multiple votes per line and so on. I'd argue there's an expectation for "blocker voting" as a whole that "+1 BetaBlocker -1 BetaFE" should work, for e.g.

I think we can only be more strict if the system is better. So for me this actually ties in with the suggestion that we should just flat out have a better system for voting. If you could vote by moving a set of pre-canned sliders, for instance, that would inarguably be a better system and this debate would be moot. But you can't. As long as humans are trying to type magic strings, they're going to screw up, and to me there's more benefit in being permissive than being strict, especially when we don't (and can't, really) do a good job of flagging up when it looked like they were trying to vote but didn't get it right.

Details: I wrote the code to accept different vote pair orders in one line consciously. It's a weird thing to do, but I don't see why we should reject it if someone does it. To me, as long as there is no other text between the vote pairs (and the parser does reject that), the intent is clear. A secondary consideration is that it would be more complex to do what is proposed here - accept both orders, and accept multiple votes per comment, but only accept one order per comment, don't allow mixing. Implementing that restriction requires additional code and I don't see what it gets us, so why do it? I honestly wouldn't be surprised if no-one ever actually does this, or only does it as a joke, but I don't see that we cause any problems by accepting it.

I entirely agree that it's possible someone might vote by accident if we allow text on the end of a line. But it's also inarguably possible that people will not vote by accident if we don't. In fact, we already know this has happened, several times. Which is worse? I think people attempting to vote with an explanation is more likely to happen than them starting a line of debate with a valid vote; all the examples of that seem pretty forced, to me. I don't think it's common for people to write "+1 BetaBlocker would be my opinion but blah blah blah". A much more natural way to write that is "I would say +1 BetaBlocker but blah blah blah", which this parser does not consider to be a vote. "BetaBlocker +1? Are you crazy?" is a better example, but would not actually be counted as a vote by this parser either, due to the question mark.

I also agree that there's kind of a slippery slope to ever-more-complicated expectations, and that's a reasonable concern. On the whole I think it's still better to be more liberal, but I do see the argument. Not sure how we can settle that except by pistols at dawn. :D

Basically, I don't think there's a perfect system for parsing votes out of a free text comment system where we know some of the comments won't be votes. The current parser isn't it, and neither is this. I think this one is a bit better. I think it would be substantially better to implement voting in a way which isn't "parsing the votes out of comment text".

OK, so I checked, and there doesn't seem to be any existing test of a vote line with trailing text. I did add two such tests in this PR: test_singleline_multivote_same and test_singleline_multivote_different. They both use a line with two valid vote string pairs, then some non-vote text, then another vote string pair, and they both check that the first two votes are counted but the third is not (since it appears after some non-vote text, i.e. we're now in the "description" part of the line).

Mhmm. We clearly have differing opinions here :bomb: :smile:

I'd say that the current voting system is easier to get right, because it is easier to make a mistake in your system. Probably the most likely is the missing whitespace after the vote, like BetaBlocker +1, definitely. I even made the same error myself in BetaBlocker +1? Are you crazy? (nice catch, btw). That shows how easy it is to miss this rule. It is definitely harder to remember than "each vote on a separate line, period".

A secondary consideration is that it would be more complex to do what is proposed here - accept both orders, and accept multiple votes per comment, but only accept one order per comment, don't allow mixing.

Actually, that's not what I meant. I meant that if we allow multiple votes per line, there should be just one way to write a vote (both a single vote and a multi-vote), no choice.

I always try to imagine how difficult it is to write the voting guide instructions for a given system. Currently, the instructions can be pretty succinct. If you allow arbitrary keyword order, but require it to be consistent in a multi-vote, the explanation gets more complex (including examples of what you can't do) and I'd rather avoid that.

(A complex explanation will also be needed for the voting system as currently proposed in this PR).

If you could vote by moving a set of pre-canned sliders
I think it would be substantially better to implement voting in a way which isn't "parsing the votes out of comment text".

Yes, yes, yes. If you can show me a way which is similarly fast to implement and can be easily transferred to a new ticketing system if Pagure gets canned, I'm all ears :wink:

There's also the option of trying to find a different approach. Here's one crazy idea. We could require all votes to be enclosed in backticks, i.e.

`BetaBlocker +1 FinalBlocker -1`, because I'm right

shown as:

BetaBlocker +1 FinalBlocker -1, because I'm right

This has the advantage of easily detecting the borders of the voting string and therefore a suffixed whitespace is not needed. It also renders the text highly visible in Markdown, so everyone can easily spot other people's votes, even when buried in paragraphs of text, and also you can easily spot a quoting error in your own post (in preview or after submitting it), because it wouldn't render as expected. With this approach I'd be much less concerned about allowing appending text after the vote. The downside is that we'd also probably need to handle code blocks, because some people would immediately want to vote like this:

```
BetaBlocker +1
FinalBlocker -1
```

And that brings more complications, because the markdown spec allows you to use 3+ backticks or tildas, not just exactly 3. We could look into using a markdown library for extracting code highlights, or we could simply say only a single or a triple backtick is supported (a reasonable requirement, imo) and parse it ourselves. (To be clear, users would still be required to start the line with the backtick(s), so the detection is quite easy. Yes, there are some edge cases in this approach... but I decided to ignore them).

Is it crazy? Or a decent idea? Tell me.

If we want to decide just between the current voting system and this PR's voting system, I strongly suggest we don't allow text after the vote. The rest is "OK" (tolerable :smile: ). But I don't intend to block this either. If you strongly feel about all the changes as proposed, we can put them it, but you'll be the one who reminds everyone with an incorrectly written vote that they need to edit their comment and why, and I'll be the one who reserves the right to say "I told you so" every time :tophat:

I always try to imagine how difficult it is to write the voting guide instructions for a given system.

See, to me this is the trick: the instructions don't have to include everything ;)

This is an opinion I got from writing wikitcms, mainly. If you read the instructions/examples on how to format a result or do various other things in the wiki, they are fairly simple and specific. But python-wikitcms actually accepts and parses more stuff than the instructions tell you, because people are people and have done various forms of crazy stuff over the years. That doesn't mean we have to change the instructions to tell you about all the crazy stuff you can actually do that will work. There's no need to encourage people. ;)

So in this case we can leave all sorts of stuff out of the instructions. We can not say anything about you being allowed to write text after a vote. We can only document one of the vote pair orders. Just because something works doesn't mean it has to be documented ;)

Still, I can probably be fine with not allowing text after the vote if that's the compromise. It is also a very simple change because it's actually already in the code, we just flip a boolean somewhere. I might actually want to look and see how difficult it would be to tell people "hey it kinda looks like you were trying to vote but it didn't work", though. Probably too hard.
Edited 4 years ago by adamwill

but you'll be the one who reminds everyone with an incorrectly written vote that they need to edit their comment and why

why bother? we're admins. I just fix it. ;)

rebased onto a563367
4 years ago

OK, so in the interests of peace and goodwill to all men, I revised this to no longer accept any non-vote text in a vote line.

Build succeeded.

BTW, on voting: just in my head, I don't think it should be that hard to implement it in Pagure, honestly. Off the top of my head the way I'd plan it would be to have a project setting that enables/disables vote functionality. If it's enabled, an admin would be able to create a vote in any issue, which would either appear as the next comment or in a single pre-defined location at the top or something. The admin would define the items for voting and the range of values that could be selected, and Pagure would show that to people viewing the issue and allow them to vote or adjust their vote. I don't think any of that ought to be terribly hard. It would obviously have to be available via an API in some way, so we could automate the creation of the votes in our case. Vote creation could even be done by magic text, I suppose...:D

BTW, on voting: just in my head, I don't think it should be that hard to implement it in Pagure

The collective wisdom at that time (when we started working on this) was that we should avoid Pagure-specific solutions, IIRC. If you think that it should be re-discussed, I'm happy to have that conversation.

OK, so in the interests of peace and goodwill to all men, I revised this to no longer accept any non-vote text in a vote line.

OK, that was my major issue. I still not thrilled about the new approach, but @lbrabec agreed to it (if I read his comments correctly) and you believe it's a good way to go, so let's keep it this way.

I'll go through the updated PR properly including the unit tests, perhaps even add some corner cases mentioned in this ticket. If I don't see any issue, I'll merge this.

I'm somewhat concerned about this. It only works because we don't have the "fe" substring anywhere else in our keywords. But if we did, or if we change it in the future, this will start causing troubles. I'd rather make this change more systematic and safer. I'll add a patch.

I went a bit overboard with my changes, but it's so much better now! :-D I couldn't add commits to Adam's branch, so I had to create a new PR with a shared branch (can be updated further, if needed). Please review #163 as a replacement for this PR, thanks.

Closed in favor of #163.

Pull-Request has been closed by kparal
4 years ago

Commit 74721fc fixes this pull-request

Pull-Request has been merged by kparal
4 years ago
Changes Summary 2
+65 -55
file changed
blockerbugs/util/pagure_bot.py
+281 -93
file changed
testing/test_pagure_bot.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.