[GitHubIssues] Support excludeDrafts and onlyDrafts #11401
Conversation
| import { GithubAuthV4Service } from './github-auth-service.js' | ||
| import { documentation, transformErrors } from './github-helpers.js' | ||
|
|
||
| const issueCountSchema = Joi.object({ |
There was a problem hiding this comment.
you seem to remove this schema while keeping the request for this later, why is it removed?
| totalCount | ||
| } | ||
| } | ||
| search(query: $query, type: ISSUE) { |
There was a problem hiding this comment.
Why did we replace repository queries with search? I have a few concerns i want to clear
Will it cost more of our rate limit?
Does search have limitations for results we might need to consider?
Also, we already have the GitHub issue custom search badge, Can we make reuse of it's code for a smaller code, should we?
There was a problem hiding this comment.
Why did we replace repository queries with search? I have a few concerns i want to clear
I didn't find any way to filter on draft flag with the repository query. Only the search one supports it as far as I have found in the doc.
Will it cost more of our rate limit?
I have made a few tests with adding { ratelimit { cost } } in the query (https://docs.github.com/en/graphql/overview/rate-limits-and-query-limits-for-the-graphql-api#returning-the-point-value-of-a-query), and I obtain a cost of 1 in any case, with the repository or the search query.
Does search have limitations for results we might need to consider?
I cannot think of any. search seems much more flexile than repoitory
If you prefer, we can limit the changes for the previously supported requests (without draft), to still use the repository query, and switch to the search query only when a draft-related parameter is specified. What do you think?
Also, we already have the GitHub issue custom search badge, Can we make reuse of it's code for a smaller code, should we?
I'm not sure. Using search here is an implementation detail, someone may find a better solution, and it would be more difficult to decouple from the other one in my opinion
| }) | ||
|
|
||
| t.create('GitHub closed pull requests excluding drafts') | ||
| .get('/issues-pr-closed/badges/shields.json?excludeDrafts=true') |
There was a problem hiding this comment.
You don't need to explicitly add a true value, including the query param will assume true value.
| .get('/issues-pr-closed/badges/shields.json?excludeDrafts=true') | |
| .get('/issues-pr-closed/badges/shields.json?excludeDrafts') |
There was a problem hiding this comment.
For some reason I don't understand, this is failing with this change. I'm getting this error when testing:
at IcedFrisbyNock._expectField (file:///shields/core/service-test-runner/icedfrisby-shields.js:85:51)
at IcedFrisbyNock.<anonymous> (file:///shields/core/service-test-runner/icedfrisby-shields.js:69:26)
at IcedFrisbyNock.<anonymous> (node_modules/icedfrisby/lib/icedfrisby.js:954:10)
at invokeNextHook (node_modules/icedfrisby/lib/icedfrisby.js:1003:24)
at /shields/node_modules/icedfrisby/lib/icedfrisby.js:1017:7
at new Promise (<anonymous>)
at IcedFrisbyNock._runHooks (node_modules/icedfrisby/lib/icedfrisby.js:976:12)
at IcedFrisbyNock.run (node_modules/icedfrisby/lib/icedfrisby.js:1276:20)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
at async Context.<anonymous> (node_modules/icedfrisby/lib/icedfrisby.js:1348:9)
Co-authored-by: jNullj <[email protected]>
3 participants
Add support for
excludeDraftsandonlyDraftsfor GitHub Issues serviceFixes #11286