Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: task limit improvement #66
feat: task limit improvement #66
Changes from 19 commits
5497d7e
f00a9c6
3e18c1b
cd96022
0b63098
58fd4da
734f97b
ad20e32
044f8a8
72cc120
0953019
2474312
d09f2d4
8bf9f6f
910ebf0
239d26e
32e9280
d10db4a
0a6bd74
8fcedd9
87e7b16
986261a
d8fead7
3827d28
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here but in the configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reasoning is that it's needed at multiple places so they just put it in the context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec states:
Shouldn't this be configurable? And even if not, putting it in the config also allows to add defaults on decode which would ensure it is properly initialized and types as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. It mentions the scope is configurable: repo, org , or network.. Dont understand what you meant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm missing something, but the config item
assignedIssueScope
can be set torepo
,org
ornetwork
, thisconfiguration
property gets filled based onassignedIssueScope
when the plugin startsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's how it behaves.. Based on
assignedIssueScope
, it will fill in theorganization
accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you use the organizations with a
search
, it is possible that the user has a private profile which will result in a422
error when querying the search API. You need to provide a fallback for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a function
getAssignedIssuesFallback
isnt it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could reproduce it, but now i am not sure what kind of fallback we can do here??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, the function
getAssignedIssuesFallback
handled it gracefully.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested with a user having a private profile, the run crashed on a 422 error. I will test again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the construct to 'await' so it will be able to catch 422 and trigger getAssignedIssuesFallback