-
Notifications
You must be signed in to change notification settings - Fork 746
refactor(amazonq): reorganize transformation history code #7843
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(amazonq): reorganize transformation history code #7843
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
| const metadata: JobMetadata = { | ||
| jobId: jobId, | ||
| projectName: transformByQState.getProjectName(), | ||
| transformationType: transformByQState.getTransformationType() ?? TransformationType.LANGUAGE_UPGRADE, |
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.
Just curious here, so what would happen for SQL conversions?
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.
Technically transformByQState.getTransformationType() should not be undefined at this point, so it will be written as "SQL Conversion" in the metadata file. But the ?? TransformationType.LANGUAGE_UPGRADE is there just in case for some odd reason it is undefined.
| ` | ||
| } | ||
|
|
||
| private async refreshJob(jobId: string, currentStatus: string, projectName: string) { |
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 are taking a lot of stuff out here and moving to a different file for this refactor. Just ensure we don't miss anything or introduce logic that wasn't covered before.
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.
Yup! No major new logic, just moved these functions to a new file where it would make a little more sense and created some helper functions to make things neater. Functionality remains the same for the entire feature.
| import { ExportResultArchiveStructure } from '../../../shared/utilities/download' | ||
| import { isFileNotFoundError } from '../../../shared/errors' | ||
|
|
||
| export interface HistoryObject { |
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 know previously these were in code whisperer constants and now they got moved to this file. Did we want to keep them in code whisperer constants still and import them for use here?
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'm leaving them here for now just because they have the most context within this file and I didn't see any interfaces defined in the constants file, but if they would be better suited in the constants file, I'd be happy to move them.
packages/core/src/codewhisperer/service/transformByQ/transformationHubViewProvider.ts
Show resolved
Hide resolved
2e29b54 to
1da46b7
Compare
1da46b7 to
c9d234b
Compare
c9d234b to
7966432
Compare
laileni-aws
left a comment
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
## Problem Job history-related code is scattered throughout various files, making it difficult to review and understand. ## Solution Centralize existing history functions in a new file and add helper functions to declutter transformation flow. Update and simplify unit tests accordingly. --- - 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](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Problem
Job history-related code is scattered throughout various files, making it difficult to review and understand.
Solution
Centralize existing history functions in a new file and add helper functions to declutter transformation flow. Update and simplify unit tests accordingly.
feature/xbranches will not be squash-merged at release time.