pagure

Clone

#934 clean up regexes a bit
Merged 7 years ago by pingou. Opened 7 years ago by mikem.
mikem/pagure regex-whitespace  into  master

file modified
+24 -11 
@@ -57,23 +57,36 @@ 
 

  

  

  DESCRIPTION = '''
 

- Pagure specific hook to add a comment to issues if the pushed commits fix them
 

+ Pagure specific hook to add a comment to issues or pull requests if the pushed
 

+ commits fix them
 

  or relate to them. This is determined based on the commit message.
 

  

- To reference an issue you need to use one of recognized keywords followed by an
 

- issue number. The number can optionally be preceded by `#` symbol.
 

+ To reference an issue/PR you need to use one of recognized keywords followed by
 

+ a reference to the issue or PR, separated by whitespace and and optional colon.
 

+ Such references can be either:
 

+ 

+  * The issue/PR number preceded by the `#` symbol
 

+  * The full URL of the issue or PR
 

+ 

+ If using the full URL, it is possible to reference issues in other projects.
 

+ 

+ The recognized keywords are:
 

+ 

+  * fix/fixed/fixes
 

+  * relate/related/relates
 

+  * merge/merges/merged
 

+ 

+ Examples:
 

+ 

+  * Fixes #21
 

+  * related: https://pagure.io/myproject/issue/32
 

+  * this commit merges #74
 

+  * Merged: https://pagure.io/myproject/pull-request/74
 

+ 

  Capitalization does not matter; neither does the colon between keyword and
 

  number.
 

  

-  * fix
 

-  * fixed
 

-  * fixes
 

-  * relate
 

-  * related
 

-  * relates
 

  

- Instead of an issue number, you can use full URL of the issue. This way it is
 

- possible to reference issues in other projects.
 

  '''

file modified
+7 -11 
@@ -15,21 +15,17 @@ 
 

  

  

  FIXES = [
 

-     re.compile(r'fixe?[sd]?:?\s*?#(\d+)', re.I),
 

-     re.compile(r'.*\s*fixe?[sd]?:?\s*?#(\d+)', re.I),
 

-     re.compile(r'fixe?[sd]?:?\s*?https?://.*/(\w+)/issue/(\d+)', re.I),
 

-     re.compile(r'.*\s*?fixe?[sd]?:?\s*?https?://.*/(\w+)/issue/(\d+)', re.I),
 

-     re.compile(r'merge?[sd]?:?\s*?#(\d+)', re.I),
 

-     re.compile(r'.*\s*merge?[sd]?:?\s*?#(\d+)', re.I),
 

-     re.compile(r'merge?[sd]?:?\s*?https?://.*/(\w+)/issue/(\d+)', re.I),
 

-     re.compile(r'.*\s*?merge?[sd]?:?\s*?https?://.*/(\w+)/issue/(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?fixe?[sd]?:?\s*?#(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?fixe?[sd]?:?\s*?https?://.*/(\w+)/(?:issue|pull-request)/(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?merge?[sd]?:?\s*?#(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?merge?[sd]?:?\s*?https?://.*/(\w+)/(?:issue|pull-request)/(\d+)', re.I),
 

  ]
 

  

  RELATES = [
 

-     re.compile(r'.*\s*relate[sd]?:?\s*?(?:to)?\s*?#(\d+)', re.I),
 

-     re.compile(r'.*\s*relate[sd]?:?\s?#(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?#(\d+)', re.I),
 

+     re.compile(r'(?:.*\s+)?relate[sd]?:?\s?#(\d+)', re.I),
 

      re.compile(
 

-         r'.*\s*relate[sd]?:?\s*?(?:to)?\s*?https?://.*/(\w+)/issue/(\d+)',
 

+         r'(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?https?://.*/(\w+)/issue/(\d+)',
 

          re.I),
 

  ]

file modified
+58 -27 
@@ -191,34 +191,65 @@ 
 

  

      def test_fixes_regex(self):
 

          ''' Test the fixes regex present in pagure.lib.link. '''
 

-         text = 'fixes     http://localhost/fork/pingou/test/issue/1'
 

-         for index, regex in enumerate(pagure.lib.link.FIXES):
 

-             if index in [2, 3]:
 

-                 self.assertNotEqual(regex.match(text), None)
 

-             else:
 

-                 self.assertEqual(regex.match(text), None)
 

- 

-         text = 'fix http://209.132.184.222/fork/pingou/test/issue/1'
 

-         for index, regex in enumerate(pagure.lib.link.FIXES):
 

-             if index in [2, 3]:
 

-                 self.assertNotEqual(regex.match(text), None)
 

-             else:
 

-                 self.assertEqual(regex.match(text), None)
 

  

-         text = 'This fixed  #5'
 

-         for index, regex in enumerate(pagure.lib.link.FIXES):
 

-             if index == 1:
 

-                 self.assertNotEqual(regex.match(text), None)
 

-             else:
 

-                 self.assertEqual(regex.match(text), None)
 

- 

-         text = 'Could this be fixes  '\
 

-             ' https://fedorahosted.org/pagure/tests2/issue/6'
 

-         for index, regex in enumerate(pagure.lib.link.FIXES):
 

-             if index == 3:
 

-                 self.assertNotEqual(regex.match(text), None)
 

-             else:
 

-                 self.assertEqual(regex.match(text), None)
 

+         # project/issue matches
 

+         def project_match(text, groups):
 

+             match = None
 

+             for regex in pagure.lib.link.FIXES:
 

+                 match = regex.match(text)
 

+                 if match:
 

+                     break
 

+             self.assertNotEqual(match, None)
 

+             self.assertEqual(len(match.groups()), 2)
 

+             self.assertEqual(match.groups(), groups)
 

+ 

+         data = [
 

+             # [string, groups]
 

+         ]
 

+ 

+         project_match('fixes     http://localhost/fork/pingou/test/issue/1',
 

+             ('test', '1'))

I am now realizing that this is a bug, we should retrieve ('pingou', 'test', 1) otherwise we will be closing a ticket on the original project instead of the fork, looks like the bug was here before but your changes are making it apparent.

 
+         project_match('fix http://209.132.184.222/fork/pingou/test/issue/1',
 

+             ('test', '1'))
 

+         project_match('Could this be fixes  '
 

+             ' https://fedorahosted.org/pagure/tests2/issue/6',
 

+             ('tests2', '6'))
 

+         project_match('merged https://pagure.io/myproject/pull-request/70',
 

+             ('myproject', '70'))
 

+         project_match('Now we merge https://pagure.io/myproject/pull-request/99',
 

+             ('myproject', '99'))
 

+ 

+         # issue matches
 

+         def issue_match(text, issue):
 

+             match = None
 

+             for regex in pagure.lib.link.FIXES:
 

+                 match = regex.match(text)
 

+                 if match:
 

+                     break
 

+             self.assertNotEqual(match, None)
 

+             self.assertEqual(len(match.groups()), 1)
 

+             self.assertEqual(match.group(1), issue)
 

+ 

+         issue_match('This fixed  #5', '5')
 

+         issue_match('Merged  #17', '17')
 

+         issue_match('Fixed:  #23', '23')
 

+         issue_match('This commit fixes:  #42', '42')
 

+         issue_match('Merge #137', '137')
 

+ 

+         # no match
 

+         def no_match(text):
 

+             match = None
 

+             for regex in pagure.lib.link.FIXES:
 

+                 match = regex.match(text)
 

+                 if match:
 

+                     break
 

+             self.assertEqual(match, None)
 

+ 

+         no_match('nowhitespacemerge: #47')
 

+         no_match('This commit unmerges #45')
 

+         no_match('Fixed 45 typos')
 

+         no_match('Fixed 4 typos')
 

+         no_match("Merge branch 'work'")
 

  

  

  if __name__ == '__main__':

.\s is equivalent to .* which doesn't seem to be intended

(?:.*\s+)? will match the empty string, or arbitrary space-separated prefixes

Existing code will match some things that were probably not intended. E.g.



p = re.compile(r'.\smerge?[sd]?:?\s*?#(\d+)', re.I)

p.match('nowhitespacemerge: #45').groups()

('45',)

p.match('This commit unmerges #45').groups()

('45',)

Just to be sure, did you run the test suite?

I'm also thinking if it might be worth adding the invalid case you suggested to make this doesn't fail in the future

I could not run it actually. Doesn't like f22.
I did test the regexes. If I can sort that out, I'd be happy to add resubmit with some test cases

I could not run it actually. Doesn't like f22.

Could you maybe catch me on IRC with the traceback?

Otherwise the tests for the regex are in:
https://pagure.io/pagure/blob/master/f/tests/test_pagure_lib_link.py

It was failing from missing deps. I manually rebuild a few fc23 packages
and got it working.

  • python2-flask-multistatic
  • python2-binaryornot

So... yeah, turns out my change does break the test, but only because
the tests depend on the index of the regex in the list (which seems wrong).

I'll update this PR with some updated tests later. Don't have much time
to look at it today.

2 new commits added

  • update+expand tests for fixes regex
  • also accept pull-request urls for fix/merge links
7 years ago

This passes the updated tests. Let me know what you think.
I carried the existing test strings over and added some new ones.
Edited 7 years ago by mikem

I am now realizing that this is a bug, we should retrieve ('pingou', 'test', 1) otherwise we will be closing a ticket on the original project instead of the fork, looks like the bug was here before but your changes are making it apparent.

The changes are looking really nice, they also showed that there was a bug in my regex.

Would you like to see about fixing them or should we do that in another PR (if so I'll merge this one)?

It looks like more than just the regex. When get_relation() searches for related issues, it seems to only consider a single repo:

repo = pagure.lib.get_project(session, reponame, user=username)
....
relation = pagure.lib.search_issues(session, repo=repo, issueid=relid)

I guess in the forks case, we'd need to call get_project() again for the other repo and adjust the search for that case. What would I need to pass to get_project() to get the correct forked repo? reponame="$user/$project"?

What would I need to pass to get_project() to get the correct forked repo?

It would need a user=username to get the fork: repo = pagure.lib.get_project(session, repo, user=username) if username is None, then it return the main project, otherwise it searches for a fork.
Edited 7 years ago by pingou

1 new commit added

  • Update pagure_hook description
7 years ago

Looked at it a little. Maybe best to pull this in and do another PR to fix handling of fork urls.

Also, updated the text in in the hook description.

Looked at it a little. Maybe best to pull this in and do another PR to fix handling of fork urls.

Ok, would you mind to rebase your branch on the top of master then?

Also, updated the text in in the hook description.

Thanks

rebased
7 years ago

Pull-Request has been merged by pingou
7 years ago
Metadata
None
No Tags
Changes Summary 3
+24 -11
file changed
pagure/hooks/pagure_hook.py
+7 -11
file changed
pagure/lib/link.py
+58 -27
file changed
tests/test_pagure_lib_link.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.