Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send base_sha and base_ref when appropriate #329

Merged
merged 7 commits into from
Dec 7, 2020
Merged

Conversation

rneatherway
Copy link
Contributor

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rneatherway
Copy link
Contributor Author

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'll test manually before merging, thanks.

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.

I'd be happier if someone else had a quick look at your first commit.

mode: util.Mode
) {
if (mode === "actions") {
const payloadObj: any = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your suggestion is cleaner so I've gone with that.

@rneatherway rneatherway merged commit 6821589 into main Dec 7, 2020
@rneatherway rneatherway deleted the robertbrignull/meta branch December 7, 2020 12:47
@github-actions github-actions bot mentioned this pull request Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants