search

jasonkuster / merge-bot

An automatic merge robot for the ASF.
http://mergebot-vm.apache.org:8080
Apache License 2.0
2 stars 5 forks source link

Mergebot PR #3: merge.py #8

Closed jasonkuster closed 8 years ago

jasonkuster commented 8 years ago

Changes to merge.py to increase functionality and integrate it with previous components of the mergebot.

Initial review should focus on structural feedback first. Once structure is solidified, test feedback should come after. Signed-off-by: Jason Kuster jason@google.com

jasonkuster commented 8 years ago

R: @bjchambers

jasonkuster commented 8 years ago

Added some tests for merge.py.

bjchambers commented 8 years ago

Looking pretty close

jasonkuster commented 8 years ago

Rework to use a logging library (hacks with sys.stdout have been removed), and rework of merge.py as we discussed. I believe this is in its final state; it'd be great to get this in and done tomorrow.

@bjchambers PTAL

jasonkuster commented 8 years ago

To view the recent changes, take a look at the below link.

https://github.com/jasonkuster/merge-bot/pull/8/files/18821e69f688ce8e8aeab12817b99d3de783480b..d5720bfcf70dc453ca281b3c0c1ab687fa1dc726

jasonkuster commented 8 years ago

@bjchambers PTAL

bjchambers commented 8 years ago

As discussed, I'm OK (but somewhat hesitant) to defer cleaning up logging to Mergebot 1.1. As a general idea, it seems to make sense to create classes for each of the kinds of loggers (with appropriate helper methods) and use those as appropriate. Eg, have a PrLogger class which can have methods to log to just the PrLogger or to the PrLogger and the higher-level merge logger, etc.

bjchambers commented 8 years ago

Some nits, then looks good for this PR (with the assumption we'll clean up the logging later)

jasonkuster commented 8 years ago

I'll write up something about the logging refactor, but for now, submitting!