Execute only changed commands #682
Conversation
There was a problem hiding this comment.
This looks amazing! I think we should drop the (now not necessary) guards against other events, to simplify the code a bit.
It would also be good if we could extract the "reparsing" to a function somewhere (maybe just freestanding) to avoid duplicating the code.
| log::debug!("skipping event, issue was {:?}", e.action); | ||
| // skip events other than opening the issue to avoid retriggering commands in the | ||
| // skip events other than opening or editing the issue to avoid retriggering commands in the |
There was a problem hiding this comment.
Hm, so if we're correctly handling things, this should be entirely removable, right? Since we'll never retrigger unless the command has been edited.
There was a problem hiding this comment.
Well I though about doing so, but there are really a lot of event that targets issues. So I think its worth to skip the whole parsing when e.g. just labels changed
|
About extracting the "repasing", I think there is generally a lot that can be refactored do avoid duplicate code. E.g. it seems that a message is parsed once in each handler, while it could be parsed only once and the result of that passed to each handler. |
|
I agree -- initially the duplication happened due to a lack of knowledge about what the right abstraction is, and I think that's still (to some extent) true -- not all of our handlers have the same handling. But I do think in this case it's pretty clear that the code is essentially identical between all the places, so deduplicating it makes sense. |
|
I can try to extract common code to a common place, but I'd rather do so in another PR. Btw are multiple command in the same comment supported ? |
|
I forget (would need to look at code). It's not an explicit goal, though would of course be a nice to have. I think this looks good now, though, so merging (and it will automatically deploy soon thereafter). |
This makes commands only be executed if they changed.
Fixes #114