search

tanium / octobot

github bot with slack and jira integration
MIT License
25 stars 16 forks source link

Add auto-reapproval on identical force-push #189

Closed matthauck closed 6 years ago

matthauck commented 6 years ago

btw. doing some manual testing right now and it's not quite working yet... :thinking:

trevershick commented 6 years ago

So I was reviewing this and I didn't see anything wrong, but then you added a commit and said Make it work for real which then made me question my existence for being since I couldn't tell it wouldn't have worked. Great. Existential angst due to a PR. At any rate, I don't see anything glaringly bad/wrong but the logic is pretty rough to follow and I think I would decompose it so that the workflow portion is more readable statements vs actual bit comparisons is_approved(review) vs review.state === 'approved'. This is just the first one I can think of. At any rate, I can't 'see' anything wrong with it. Of course this is all based on it working...

matthauck commented 6 years ago

Haha. I didn't see anything wrong in the first stab at it either. But manual testing with actual responses from github api exposed some flaws. :)

re. readability, point well taken. It is a little ugly. I really wish you could do multiple if let(...) statements without so much needless nesting. :disappointed:. I'll see if I can improve that section a bit.

trevershick commented 6 years ago

multiple if let(...) => tuples I think. :) . although that can get ugly quick if you have long var names or you're calling functions.

if let (Some(a), Some(b)) = (expr X, expr Y)

It looks better. If it's working :shipit:

matthauck commented 6 years ago

cool, thanks. mind re-approving?