-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(amazonq): /doc: add support for infrastructure diagrams #1
base: master
Are you sure you want to change the base?
Conversation
b64edd6
to
4a69a9e
Compare
f536bac
to
341ca89
Compare
@@ -363,7 +364,9 @@ export class PrepareCodeGenState implements SessionState { | |||
this.config.workspaceRoots, | |||
this.config.workspaceFolders, | |||
action.telemetry, | |||
span | |||
span, | |||
new AdmZip(), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
thanks. Updated
341ca89
to
99a05a3
Compare
99a05a3
to
fb11511
Compare
function isInfraDiagramFile(relativePath: string) { | ||
return ( | ||
relativePath.toLowerCase() === 'infra.dot' || | ||
relativePath.toLowerCase().endsWith('/infra.dot') || |
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.
relativeFilePath on windows might use backwards slashes
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.
updated
a832ae6
to
98c2fa2
Compare
98c2fa2
to
ad1b18a
Compare
@@ -26,6 +30,15 @@ export async function checkForDevFile(root: string) { | |||
return hasDevFile | |||
} | |||
|
|||
function isInfraDiagramFile(relativePath: string) { | |||
return ( | |||
relativePath.toLowerCase().endsWith('docs/infra.dot') || |
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.
path.join
could be used here instead
// We only respect gitignore file rules if useAutoBuildFeature is on, this is to avoid dropping necessary files for building the code (e.g. png files imported in js code) | ||
const files = await collectFiles(repoRootPaths, workspaceFolders, true, maxRepoSizeBytes, !useAutoBuildFeature) | ||
if (!useAutoBuildFeature) { | ||
if (isIncludeInfraDiagram) { |
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.
is there a way we could avoid the additional special case logic here? could we either a) remove svg from the exclude patterns, or b) pass in true for defaultExcludePatterns (like /dev does with useAutoBuildFeature). with the addition of isIncludeInfraDiagram its hard to follow all the different scenarios
} | ||
export type CollectFilesFileFilter = (relativePath: string) => boolean // returns true if file should be filtered out | ||
|
||
export async function collectFiles2( |
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.
The name of this function doesn't add value, why is there a second collect files function? take a look at naming section in docs/CODE_GUIDELINES.md
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.
It looks like the intention here is to have collectFiles use a labeled options object instead of individual parameters, if thats the case, perhaps collectFiles itself should be changed instead of adding a new function
Amazon Q /doc: Add support for infrastructure diagrams
feature/x
branches will not be squash-merged at release time.