-
Notifications
You must be signed in to change notification settings - Fork 28
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
Migrate featureFlags/getFeatureFlag and graphql/getCurrentUserCodySubscription #2632
Conversation
@@ -0,0 +1,18 @@ | |||
package com.sourcegraph.cody.agent.protocol | |||
|
|||
object GetCurrentUserCodySubscription { |
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.
We can consider renaming this class.
We should consider options to automatically generate the values of the enums.
However, I think we can simply remove what is redundant now. It will be no worse. Then, having less code it will be easier to improve the generation.
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 we should put it in protocol_extensions
package.
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.
Also, if we are only using two values from each enum we could consider using extension methods instead:
fun CurrentUserCodySubscription.isProPlan() {
return plan == "PRO"
}
fun CurrentUserCodySubscription.isPendingStatus() {
return status == "PENDING"
}
and then on the call site e.g.:
currentUserCodySubscription.isProPlan()
instead of
currentUserCodySubscription.status == GetCurrentUserCodySubscription.Status.PENDING
Then we won't need custom objects like GetCurrentUserCodySubscription
. But it is also fine as it is.
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.
makes sense ! thanks
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.
fixed!
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.
LGTM
This PR is a part of the protocol migration. Some endpoints are written by hand. We are switching to the protocol generated from Cody. In this PR: - [Migrate textDocument/*](c0a2082) <!-- start git-machete generated --> # Based on PR #2632 ## Full chain of PRs as of 2024-11-13 * PR #2633: `mkondratek/chore/migrate-api-part-2` ➔ `mkondratek/chore/migrate-api-part-1` * PR #2632: `mkondratek/chore/migrate-api-part-1` ➔ `mkondratek/chore/update-deps` * PR #2630: `mkondratek/chore/update-deps` ➔ `main` <!-- end git-machete generated --> ## Test plan - Verified with debugger - the proper values are sent - Tested with current selection in chat, tested with autocompletion, tested with inline edits - works
This PR is a part of the protocol migration. Some endpoints are written by hand. We are switching to the protocol generated from Cody. In this PR:
Based on PR #2630
Full chain of PRs as of 2024-11-13
mkondratek/chore/migrate-api-part-1
➔mkondratek/chore/update-deps
mkondratek/chore/update-deps
➔main
Test plan