Send base_sha and base_ref when appropriate - #329
rneatherway merged 7 commits into
Conversation
Sorry, something went wrong.
robertbrignull
left a comment
There was a problem hiding this comment.
Version checking and payload construction LGTM. Probably best to be testing manually too but if you've done that then I'm fully happy.
I realise I wrote a fair bit of the version checking code. Do you think we should get someone else to review or is it good as it is? Perhaps @chrisgavin if they have time as they wrote the original version checking code.
Sorry, something went wrong.
I'll test manually before merging, thanks.
I'd be happier if someone else had a quick look at your first commit. |
Sorry, something went wrong.
| mode: util.Mode | ||
| ) { | ||
| if (mode === "actions") { | ||
| const payloadObj: any = { |
There was a problem hiding this comment.
@robertbrignull I wasn't really happy with this type annotation. Does it make sense to add a union type for this between the old and new versions?
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, I'm not sure. The union type would be a bit messy so it wouldn't really give much other than satisfying the type checker.
For now you could get around it another way, by removing the any here and adding type annotations to the base_ref and base_sha fields like
base_ref: undefined as undefined | string,
base_sha: undefined as undefined | string,Depends if you think that's better.
We may end up have to give buildPayload an explicit return type later down the road anyway. One of the eslint rules we want to eventually enable requires all exported functions to have an explicit return type.
However for now I don't mind.
Sorry, something went wrong.
There was a problem hiding this comment.
I think your suggestion is cleaner so I've gone with that.
Sorry, something went wrong.
Merge / deployment checklist