Add repository parameter #19
Conversation
|
not sure if I should include the changes that showed up in dist... I can drop the commit if you want. |
yeah could you please drop it? The build script will take care of updating it when I cut a new release |
This reverts commit 63ff1ee.
|
Okay, I removed the dist changes. |
index.js
Outdated
| const repo = core.getInput('repo') | ||
|
|
||
| var repoValue | ||
| if (!repo) { | ||
| repoValue = context.repo | ||
| } else { | ||
| const [owner, repo] = repo.split('/') | ||
| repoValue = {owner, repo} | ||
| } | ||
|
|
||
| const octokit = new GitHub(token) | ||
|
|
||
| await octokit.pulls.createReview({ | ||
| ...context.repo, | ||
| ...repoValue, |
There was a problem hiding this comment.
Hey, out of respect for your time and since I saw a few areas to improve, let me suggest an implementation here:
const repoString = core.getInput('repo')
let repoObject
if (repoString) {
const [owner, repo] = repoString.split('/')
repoObject = { owner, repo }
} else {
repoObject = context.repo
}
const octokit = new GitHub(token)
await octokit.pulls.createReview({
...repoObject,Main changes:
- use
letinstead ofvar, which is more explicit - get around the awkward repo naming (it's not our fault "repo" can mean repository name or "owner/repo" string) by being explicit about it
- conditionals read easier if the positive condition comes first
There was a problem hiding this comment.
Thanks, I don't program in JavaScript except for very occasional contributions like this.
I had to read about let vs var. let is more scoped than var but just as explicit. It is scoped to the exact block not just the function you are in. scoping is good for many reasons.
There was a problem hiding this comment.
if you're interested, let is more explicit than var in that you expect the value of let to change, while if you have an unchangable value you use const. var is used for both purposes
|
Thank you for your contribution :) https://github.com/juliangruber/approve-pull-request-action/releases/tag/v1.1.0 |
Fixes #18