Skip to content

Conversation

@zixlin7
Copy link
Contributor

@zixlin7 zixlin7 commented Mar 26, 2025

Problem

customization support for inline through language server:

  • use lsp to get list of customizations
  • notify lsp of the selected customization

Solution

it won't be e2e functional until aws/language-servers#882 is released but it doesn't break the current inline


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@zixlin7 zixlin7 marked this pull request as ready for review March 27, 2025 00:29
@zixlin7 zixlin7 requested review from a team as code owners March 27, 2025 00:29

// This command is only declared and registered in Amazon Q if Q exists

export const connectCustomizationHandler =
Copy link
Contributor

Choose a reason for hiding this comment

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

is the only difference of this implementation is that it plumbs down the client into select customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's right

@zixlin7 zixlin7 requested a review from jpinkney-aws March 28, 2025 00:06
Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

As a thought experiment: instead of passing down the language client through everything, what if we passed through the available customization instead? Would that make the code easier to understand/break up separation of concerns between the lsp calls and what's currently in the code?

@zixlin7
Copy link
Contributor Author

zixlin7 commented Mar 28, 2025

As a thought experiment: instead of passing down the language client through everything, what if we passed through the available customization instead? Would that make the code easier to understand/break up separation of concerns between the lsp calls and what's currently in the code?

hmm what we can do is pass in those two functions to get customizations and send notification about the selected one. But I feel like passing client itself might be leaner.

@zixlin7 zixlin7 requested a review from jpinkney-aws March 28, 2025 18:01
@Will-ShaoHua
Copy link
Contributor

Will-ShaoHua commented Mar 28, 2025

Zoe maybe we can take this chance to make custom related thing to a class CustomizationService and abstract all the LSP detail there? I feel it will make it easier for caller to use, also easier to test against, currently everything is first class function and it makes it hard to unit test

@Will-ShaoHua
Copy link
Contributor

it would be easy to use to have a class

class CustomizationService {
      construcotr(private lspClient: Client) {}
      
      async switchCustomization() {
              this.lsClient.sendNotification(...)
      }

      async listAvailableCustomizations() {
             await this.lspClient.listCustomization()
             ....
     }

     .....
}

@Will-ShaoHua
Copy link
Contributor

i wonder is there any concern we don't do migration work in feature branch? @jpinkney-aws I feel doing these changes in feature branch will make us easier to make large surface refactring with out too much operational concerns.

}

export async function notifyNewCustomizations() {
export async function notifyNewCustomizations(client?: LanguageClient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yea let's wrap all these things into a class and have the client in one place instead of passing the client in all function calls

@zixlin7
Copy link
Contributor Author

zixlin7 commented Mar 28, 2025

Zoe maybe we can take this chance to make custom related thing to a class CustomizationService and abstract all the LSP detail there? I feel it will make it easier for caller to use, also easier to test against, currently everything is first class function and it makes it hard to unit test

I think this refactor might be better after we deprecate the old code path. The current implementation might not be the prettiest but it should be pretty safe.

@zixlin7 zixlin7 requested a review from Will-ShaoHua March 28, 2025 21:52
@Will-ShaoHua
Copy link
Contributor

i kinda prefer refactoring them in one shot when we're writing the new LSP path as they're not yet in use in PROD so that we don't have much concern but either one works for me. But to fit in the current project planing and timeline, it makes sense to do refactoring afterward

Will-ShaoHua
Will-ShaoHua previously approved these changes Mar 28, 2025
@zixlin7
Copy link
Contributor Author

zixlin7 commented Mar 31, 2025

i kinda prefer refactoring them in one shot when we're writing the new LSP path as they're not yet in use in PROD so that we don't have much concern but either one works for me. But to fit in the current project planing and timeline, it makes sense to do refactoring afterward

created a new class refactored from the original utils and kept the original as is we can deprecate it later. Note with this approach the duplication linter fail is expected.

@jpinkney-aws
Copy link
Contributor

You can actually suppress the duplicate code linter with:

// jscpd:ignore-start
// jscpd:ignore-end

it might make sense in this case -- since the old implementation is just going to be dropped when we release this through flare. Maybe make an issue somewhere on github? (or internal?) to keep track that theres duplicate implementations, since if anyone wants to change customizations they need to change it in both places

I'm also wondering if you can just export the function calls from the customizations in core and then call those functions in your class? that way if anyone changes the implementations for now, your class would still work? WDYT?

@zixlin7
Copy link
Contributor Author

zixlin7 commented Apr 1, 2025

thanks! I will create an issue internally to track that the deprecation of the old file, most of these haven't been changed for a long time I don't see a strong need to export them into the class as it's cleaner this way and it gives us more motivation to deprecate instead of having the implementation in two places. Let me know if you a strong preference.

You can actually suppress the duplicate code linter with:

// jscpd:ignore-start
// jscpd:ignore-end

it might make sense in this case -- since the old implementation is just going to be dropped when we release this through flare. Maybe make an issue somewhere on github? (or internal?) to keep track that theres duplicate implementations, since if anyone wants to change customizations they need to change it in both places

I'm also wondering if you can just export the function calls from the customizations in core and then call those functions in your class? that way if anyone changes the implementations for now, your class would still work? WDYT?

@zixlin7 zixlin7 marked this pull request as draft April 8, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants