-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: adapt for latest @octokit/types #66
Conversation
package-lock was the issue
src/compose-create-pull-request.ts
Outdated
owner, | ||
repo, | ||
}); | ||
const hasFork = forks.data.find( | ||
(fork) => fork.owner.login === user.data.login | ||
(fork) => fork!.owner!.login === user.data.login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the right approach is since if you use fork?.owner?.login
code coverage decreases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first exclamation mark is unnecessary. I'm not sure how a fork can get into a state where fork.owner
can be null but I guess there is one. I'd change the code above to
(fork) => fork!.owner!.login === user.data.login | |
/* istanbul ignore next - fork owner can be null, but we don't test that */ | |
(fork) => fork.owner?.login === user.data.login |
Alternatively we could add a test for the case when fork.owner
is null, but I don't think it's worth it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I left two suggestions to bring bring it over the finish line :)
src/compose-create-pull-request.ts
Outdated
owner, | ||
repo, | ||
}); | ||
const hasFork = forks.data.find( | ||
(fork) => fork.owner.login === user.data.login | ||
(fork) => fork!.owner!.login === user.data.login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first exclamation mark is unnecessary. I'm not sure how a fork can get into a state where fork.owner
can be null but I guess there is one. I'd change the code above to
(fork) => fork!.owner!.login === user.data.login | |
/* istanbul ignore next - fork owner can be null, but we don't test that */ | |
(fork) => fork.owner?.login === user.data.login |
Alternatively we could add a test for the case when fork.owner
is null, but I don't think it's worth it
src/compose-create-pull-request.ts
Outdated
owner, | ||
repo, | ||
sha: base, | ||
per_page: 1, | ||
}); | ||
state.latestCommitSha = latestCommit.sha; | ||
state.latestCommitSha = latestCommit.sha as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update package-lock.json
so that @octokit/types
6.3.0+ is used? Then you can remove the as string
state.latestCommitSha = latestCommit.sha as string; | |
state.latestCommitSha = latestCommit.sha; |
import type { Endpoints } from "@octokit/types"; | ||
|
||
export type TreeParameter = Endpoints["POST /repos/:owner/:repo/git/trees"]["parameters"]["tree"]; | ||
export type TreeParameter = Endpoints["POST /repos/{owner}/{repo}/git/trees"]["parameters"]["tree"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add @octokit/types
as a direct dependency of this package, as we use it in the code, that will make it more predictable. And I've seen yarn
users having problems when using sub-dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mange tak!
🎉 This PR is included in version 3.9.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Selv tak! |
Package-lock had locked
@octokit/types
to version5.x.x
...fixes #65