patch: fix octokit api #940
Conversation
There was a problem hiding this comment.
It was failing because of new api design of octokit.
There was a problem hiding this comment.
It's good to include such information in the PR summary, imo.
| @@ -213,10 +212,17 @@ async function main() { | |||
| const emptyLabelPRs = prByLabels[''] || []; | |||
There was a problem hiding this comment.
What about this part?
I mentioned it in the Slack - won't this be always empty if we have a CI check for labels?
There was a problem hiding this comment.
we added the labels check recently, surely there will be some pr which doesn't have any labels yet. we can remove this after this release imo.
| return pr.title + " (#" + pr.number + ")"; | ||
| } | ||
| else { | ||
| return pr.data.title + " (#" + pr.data.number + ")"; |
There was a problem hiding this comment.
Does this work? (Just confirmation)
I can see .data field was removed at other plances in this PR.
There was a problem hiding this comment.
It does work, the new api is very confusing.
.data field is only present in pr objects that doesn't have any labels.
refactor: add .includes() Co-authored-by: Shunsuke Watanabe <shunsukesan.tech@gmail.com>
|
@gitofdeepanshu You need to change two lines not only 1 line. |
| const testsPRs = prByLabels[TESTS_CHANGES_LABEL] || []; | ||
| const ciPRs = prByLabels[CI_CHANGES_LABEL] || []; | ||
| const otherPRs = prByLabels[OTHER_CHANGES_LABEL] || []; |
There was a problem hiding this comment.
I'd suggest to go with a simpler solution - you have runtime, client and other.
other is everything - runtime - client (set difference operation).
There was a problem hiding this comment.
other = everything - runtime - client won't work because this will cause some of the pr which have more than 1 label to be included multiple times.
(example: let's suppose a Pr (123) has 3 tags shiden, runtime and shibuya)
This pr will be included in runtime and 2 times in other.
Of course this can be managed but that would not be a simpler solution (which is our objective).
Current solution is good enough for now imo.
There was a problem hiding this comment.
Yes, it's possible for a single PR to have multiple labels and that is fine.
It was also possible before.
The example I gave will work, not sure how you see it not working?
Iterate over all PRs and make sure that it's not included in neither of the client or runtime lists.
And I'm sorry but I won't approve this - the code highlighted in this comment is a clear anti-pattern.
It has unnecessary complexity, and is not resistant to addition of new labels (which isn't important for this example but it is in general).
There was a problem hiding this comment.
It was also possible before.
Earlier only the PRs that had runtime, client, breaksapi or empty label were included (not all), so if I had put a test label, it wouldn't have got included.
Iterate over all PRs and make sure that it's not included in neither of the client or runtime lists.
We also have to make sure that a PR doesn't get include twice which has different labels, For example: If a PR has test and ci label, then we have to make sure it only gets included once.
Now it has all the functionality that you wanted.
| // filters out the PR that has a tag `client` or `runtime` and returns all the remaining PRs | ||
| function PRfilter(prLabels: any) { |
There was a problem hiding this comment.
Nit-picky general comment - try to keep the style & approach consistent.
The formatting is off compared to other functions and e.g. below where consts are used, you use let.
| prLabels[""].forEach(element => { | ||
| if (included_pr_number.includes(element.data.number)) { | ||
| // do nothing | ||
| } |
There was a problem hiding this comment.
filter should be used in such places
There was a problem hiding this comment.
makes sense, I will make a patch file.
Pull Request Summary
fix
generate-release-body.tsfile to allow printing ofprinformation according to label.Broke due to Octokit API update.