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

Download 934.patch

.\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

This passes the updated tests. Let me know what you think.
I carried the existing test strings over and added some new ones.

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.

1 new commit added

  • Update pagure_hook description

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

Pull-Request has been merged by pingou

Metadata