Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix exported types conflicting with agent protocol #5794

Merged
merged 4 commits into from
Oct 4, 2024
Merged

Fix exported types conflicting with agent protocol #5794

merged 4 commits into from
Oct 4, 2024

Conversation

pkukielka
Copy link
Contributor

Test plan

Compile JetBrains repo with CODY_DIR set to checked out code from this branch.
There should be no errors.

@pkukielka pkukielka force-pushed the pkukielka/fix-conflicting-exported-iypes branch from 8ff16fc to a928f98 Compare October 3, 2024 15:21
This was referenced Oct 3, 2024
Closed
Merged
dominiccooney
dominiccooney reviewed Oct 4, 2024
View reviewed changes
...dings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/AuthStatus.kt Outdated

// TODO: This was edited manually due to definiencies in the codegen.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling, deficiencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not right: definiencies -> deficiencies

dominiccooney
dominiccooney approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock because this is part of a multi-sided patch to unblock JetBrains release.

However we are compounding tech debt here, which is not great. I'm concerned that there is no test plan, so how will we ensure this doesn't break when CODY-3089 is fixed? Could you:

  • Add a test plan
  • Mention TODO(CODY-3089) in your comment, and add some details about what is hand-edited here
  • Drop a note in CODY-3089 pointing to this PR and noting that we need to run the test plan after switching back to the generated bindings.

@pkukielka
Copy link
Contributor Author

Approving to unblock because this is part of a multi-sided patch to unblock JetBrains release.

However we are compounding tech debt here, which is not great. I'm concerned that there is no test plan, so how will we ensure this doesn't break when CODY-3089 is fixed? Could you:

  • Add a test plan
  • Mention TODO(CODY-3089) in your comment, and add some details about what is hand-edited here
  • Drop a note in CODY-3089 pointing to this PR and noting that we need to run the test plan after switching back to the generated bindings.
  1. I'm not sure what testplan it should be.
    TBH this would and should be caught by automatic binding regeneration.
    We ran into this issue because it was disabled.
  2. I already mentioned CODY-3089 in my comment before but I made it more explicit now.
  3. I added a note in linear (and changed it title to be more truthful) 👍

@pkukielka pkukielka force-pushed the pkukielka/fix-conflicting-exported-iypes branch from d21723d to 2935f5b Compare October 4, 2024 07:22
@pkukielka pkukielka force-pushed the pkukielka/fix-conflicting-exported-iypes branch from 2935f5b to 73065dd Compare October 4, 2024 07:29
@dominiccooney
Copy link
Contributor

Wow, JetBrains FYI did its thing: https://github.com/sourcegraph/cody/actions/runs/11175844736/job/31067972094?pr=5794

@pkukielka
Copy link
Contributor Author

Wow, JetBrains FYI did its thing: https://github.com/sourcegraph/cody/actions/runs/11175844736/job/31067972094?pr=5794

This requires fix on the jetbrains side, I will merge this PR and soon raise a fix with a cody commit bump.

@pkukielka pkukielka merged commit 656b919 into main Oct 4, 2024
17 of 19 checks passed
@pkukielka pkukielka deleted the pkukielka/fix-conflicting-exported-iypes branch October 4, 2024 11:13
pkukielka added a commit to sourcegraph/jetbrains that referenced this pull request Nov 25, 2024
@pkukielka
Switch to webview-based sign-in flow (#2382)
e07fe78 
Fixes [CODY-4347](https://linear.app/sourcegraph/issue/CODY-4347)

## Test plan

Full QA, I will add more instructions later.

Depends on:
1. sourcegraph/cody#5794
2. sourcegraph/cody#5918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiccooney dominiccooney dominiccooney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pkukielka @dominiccooney