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.

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

refactor(core): Manifest/LSP fetching should use httpResourceFetcher #6411

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

jpinkney-aws
Copy link
Contributor

Problem

Now that we've refactored the httpResourceFetcher a little bit we can use that for fetching both the manifest and the language server

Solution


  • 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.

@jpinkney-aws jpinkney-aws requested a review from a team as a code owner January 22, 2025 15:54
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.

@jpinkney-aws jpinkney-aws marked this pull request as draft January 22, 2025 18:18
@jpinkney-aws
Copy link
Contributor Author

Just moving this to draft, the web tests are failing because we currently stub out the httpResourceFetcher on web since there was previously node specific code. I just need to re-enable that

@@ -133,50 +133,6 @@ export class HttpResourceFetcher implements ResourceFetcher<Response> {
}
}

export class RetryableResourceFetcher extends HttpResourceFetcher {
Copy link
Contributor

@justinmk3 justinmk3 Jan 23, 2025

Choose a reason for hiding this comment

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

Nice that we can eliminate this! Generally better to have composable concepts, or at least flexible classes, instead of specific subclasses that implement particular compositions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was one of the motivations for doing all this httpResourceFetcher stuff

@jpinkney-aws jpinkney-aws marked this pull request as ready for review January 24, 2025 21:40
@jpinkney-aws jpinkney-aws merged commit 41e35ff into aws:feature/amazonqLSP Jan 24, 2025
17 checks passed
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