[GitHubIssues] Support excludeDrafts and onlyDrafts#11401
[GitHubIssues] Support excludeDrafts and onlyDrafts#11401feloy wants to merge 4 commits intobadges:masterfrom
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
There was a problem hiding this comment.
Thank you for the detailed response, sorry for taking so long to reply.
I think we are good on using search here as the cost is similar, if in the future we see we can save up using a condition for which api call to make we can use a follow up pr.
please also see the new comments regarding simplifying this by separation of PR and issues.
| }) | ||
|
|
||
| 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)
There was a problem hiding this comment.
I believe the schema would need to change as well: excludeDrafts: Joi.equal('')
Co-authored-by: jNullj <15849761+jNullj@users.noreply.github.com>
|
One thing I'm wondering is whether draft issues are even a thing? If not, I don't think we should be introducing options or terminology (i.e. Perhaps the way forward here would be to split the
|
|
That makes a lot of sense to me. |
3 participants
Add support for
excludeDraftsandonlyDraftsfor GitHub Issues serviceFixes #11286