-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
✅ Deploy Preview for ubiquibot-staging ready!
To edit notification comments on pull requests, go to your Netlify site 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.
Looks pretty good to me although I don't deeply understand all this code's context.
@pavlovcik this PR puts all those shared constants in config which means they can be configured by |
pavlocik asked, #748 (comment) |
Why is it a bad idea to let our partners configure these things? |
Permit base url and bot delay probably shouldn't be modified, other things are okay I guess. |
src/utils/private.ts
Outdated
@@ -161,6 +161,13 @@ export const getWideConfig = async (context: Context) => { | |||
openAIKey: mergedConfigData.openAIKey, | |||
openAITokenLimit: mergedConfigData.openAITokenLimit, | |||
staleBountyTime: mergedConfigData.staleBountyTime, | |||
startCommandEnabled: mergedConfigData.startCommandEnabled, | |||
defaultTimeRangeForMaxIssue: mergedConfigData.defaultTimeRangeForMaxIssue, |
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.
timeRangeForMaxIssue is a confusing name, I can't really tell what it means just from the name.
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 is the same name as original in shared.ts
suggestions are welcome if wey want it Changed 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.
timeRangeForMaxIssue is a confusing name, I can't really tell what it means just from the name.
What does it do? Let's rename it to something more expressive.
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.
It's used when checking if a bounty hunter can assign a new issue and it checks how many issues is already assigned to them and how many pull requests are approved or with no reviews but X time old. TimeRangeForMaxIssue
is X.
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.
What should we rename it to?
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 where the gpt /ask would come in handy for all to see
/ask can you provide some names for timeRangeForMaxIssue var
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.
timeRangeForMaxIssue is a confusing name, I can't really tell what it means just from the name.
Certainly! Here is a list of similar names for the variable "timeRangeForMaxIssue":
maxIssueTimeRange
issueTimeRangeMax
maxIssueDuration
timeFrameForMaxProblem
peakIssueTimeSpan
maximumIssueTimeFrame
issuePeakWindow
peakProblemTimeRange
maxIssuePeriod
timeSpanForPeakIssues
But they could use the defacto value. The thing is getting rid of |
Well the base url is just an RPC endpoint. I dont see the issue with partners setting their own dedicated RPC endpoints if they really need it for some reason. What is bot delay for? |
It's not RPC endpoint, it is the claim url
it seems it's for the telegram feature, as you said we can remove this feature but if someone sets it to a large number then the bot will timeout. |
Whitelabeling the claims portal seems pretty fancy but perhaps not in our best interests. It can be a secret config setting lol
We should completely wipe that stuff then it all seems like problematic code. |
The telegram stuff seems out of scope for this PR, needs greater refactor |
|
@wannacfuture might want to help pavlovcik debug and Review this simple PR? |
Would you mind resolving the conflicts? Let's get this merged before more config conflicts occur. @pavlovcik @molecula451 , Please post a QA. |
I do not see an issue with conflicts. The only conflicts shows it's |
6df0b47
to
c6f79f5
Compare
Resolves #790
Quality Assurance: