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

fix(formatter): return "No comments" if not found comments #7

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

azu
Copy link
Owner

@azu azu commented Oct 31, 2019

$ GH_TOKEN=$GITHUB_TOKEN ./bin/cmd.js --repo 'azu/get-github-pr-review-comments' --projectRoot . e452b8ab4a09ab42d5dee1ba96cb34bc2f676822
No comments

formatter should handle empty comment data.

fix #6

@azu azu merged commit 4a7637d into master Oct 31, 2019
@azu azu deleted the fix-data-undeinfed branch October 31, 2019 01:13
@markburns
Copy link

Hey @azu thanks for the fix for issue #6. Actually this introduces slightly different behaviour to what I'd expect.

Now if there's a local commit that is not on the remote, the entirety of the output is just "No comments" which isn't exactly true, because if you then push, then you get a list of all the previous comments again.

What I'd expect in both scenarios is for it to list the same list of comments.

@azu
Copy link
Owner Author

azu commented Oct 31, 2019

Using SHA is tricky reason.

I want to get pull request url from current branch name or SHA, but I do not know such API.
If GitHub provide suitable API, We can improve this search logic.

getPR({ owner, repo, commitSha }) {
const repoPath = this._createRepoString({ owner, repo });
return this.github.search.issues({
q: `repo:${repoPath} type:pr ${commitSha}`
});
}

Maybe, use oldtest commit SHA(that is pushed) instead of git rev-parse HEAD resolve this issue.

@markburns
Copy link

Ah I see. So I presume you could iteratively go back through SHAs until you get a result? Sorry I haven’t looked at your code in detail.

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.

TypeError: Cannot read property 'data' of undefined
2 participants