roundabout icon indicating copy to clipboard operation
roundabout copied to clipboard

Logging issue?

Open PerilousApricot opened this issue 13 years ago • 4 comments

Hello,

Trying to install from master (which may be my mistake), I end up with this:

[dmwm] /afs/cern.ch/user/m/meloam/roundabout > python2.6 roundabout --config_file=roundabout.cfg run No handlers could be found for logger "github2.request" [13-09-2011 11:10:45] ERROR: Unknown error: nothing to repeat

And it dies. Do I have something misconfigured?

Thanks

PerilousApricot avatar Sep 13 '11 09:09 PerilousApricot

So, I hunted this more ... the try/except wrapping around the roundabout.main.main() was hiding what the problem was. Turns out, it's not a logging thing. I changed the lgtm string to "+1", which blows up at:

Traceback (most recent call last): File "bin/roundabout", line 25, in roundabout.main.main(args[0], options) File "/afs/cern.ch/user/m/meloam/roundabout/roundabout/main.py", line 42, in main run(config) File "/afs/cern.ch/user/m/meloam/roundabout/roundabout/main.py", line 67, in run if p.lgtm(github.approvers)] File "/afs/cern.ch/user/m/meloam/roundabout/roundabout/github/client.py", line 134, in lgtm lgtm_re = re.compile("^%s$" % self.lgtm_text, re.I) File "/usr/lib64/python2.6/re.py", line 190, in compile return _compile(pattern, flags) File "/usr/lib64/python2.6/re.py", line 245, in _compile raise error, v # invalid expression sre_constants.error: nothing to repeat

Obnoxiously, apparently sre_constants.error doesn't inherit from Exception, so the try/except clause in main() didn't match, so the error was being eaten. Is it necessary to have a regular expression in github.client? If not, i'll make a patch for you.

Thanks

PerilousApricot avatar Sep 13 '11 09:09 PerilousApricot

You're right, case-insensitivity should be handled better there perhaps by replacing the lgtm comparison with: https://gist.github.com/7e32d9fe5f8d183c92df

ChristopherMacGown avatar Sep 13 '11 17:09 ChristopherMacGown

Well, it's not necessarily a case-insensitivity problem .. the string from the user gets passed into a regex. I had the string "+1", which got substituted to "^+1$" in the regex, which doesn't compile. I guess it's up to you if you want the strings to be regexes or regular strings. The pull request I made escapes the user strings so users can use arbitrary strings as lgtm strings

PerilousApricot avatar Sep 13 '11 21:09 PerilousApricot

I used a regex for the check because I wanted case insensitivity. The LGTM is only counted if the entire string is the LGTM token… so the regex is kind of useless.

ChristopherMacGown avatar Sep 14 '11 01:09 ChristopherMacGown