-
Notifications
You must be signed in to change notification settings - Fork 74
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
Support customization process part2 #1967
Support customization process part2 #1967
Conversation
"clean": "rimraf dist dist-browser dist-esm test-dist temp types *.tgz *.log", | ||
"execute:samples": "echo skipped", | ||
"extract-api": "rimraf review && mkdirp ./review && api-extractor run --local", | ||
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\" \"samples-dev/**/*.ts\"", | ||
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\" ", |
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.
Small fix when there is no sample folder included.
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 this trying to completely ignore the samples-dev folder?
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.
This is the small fix, sample folder should be ignored if there is no samples generated.
Should not be ignored if there is samples generated.
Line 34 in 2b5698f
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\" \"test/**/*.ts\" \"samples-dev/**/*.ts\"", |
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.
can you point to me where you add this check in the codegen side ?
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.
Below cases are covered in our smoke/integration test(T - true, F - false, U - undefined):
|
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.
Not sure if we by default geenrateTest and generateSample is good or not ? given the sample quality and we might have problem to compiling the code in CI.
"clean": "rimraf dist dist-browser dist-esm test-dist temp types *.tgz *.log", | ||
"execute:samples": "echo skipped", | ||
"extract-api": "rimraf review && mkdirp ./review && api-extractor run --local", | ||
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\" \"samples-dev/**/*.ts\"", | ||
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"*.{js,json}\" ", |
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 this trying to completely ignore the samples-dev folder?
packages/typespec-test/test/loadtesting_modular/generated/typespec-ts/tsconfig.json
Show resolved
Hide resolved
const { generateTest, packageDetails } = model.options || {}; | ||
let { generateTest, packageDetails, isModularLibrary } = model.options || {}; | ||
// Take the undefined as true by default | ||
generateTest = generateTest === true || generateTest === undefined; |
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.
Can we have some automatical check if the tests exist or not ?
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 do this automatical check in emitter side, https://github.com/Azure/autorest.typescript/pull/1967/files#diff-3d0e467e1b9edbc8377b86fbeaac71ce5bc0ddc8260bee5e56a777360cf19398R236-R242.
In common layer we also need to check if we need to generate test relevant stuff so we have the dependency of generateTest
.
const project = new Project(); | ||
const config = { | ||
$schema: | ||
"https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
mainEntryPointFilePath: `./types${generateTest ? "/src" : ""}/index.d.ts`, | ||
mainEntryPointFilePath: `./types${ | ||
generateTest || isModularLibrary ? "/src" : "" |
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.
why isModularLibrary can have impact on this mainEntryPointFilePath ?
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.
For modular the generated index.d.ts
would be in types/src
folder.
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.
even if we don't generate tests and samples ?
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.
Yes, this is because in modular tsconfig.json we by default add below option which would always intro a src folder in dist-ems folder.
"rootDir": "."
The default behavior of generateSample(not generating samples) is not changed, I enable the |
const hasPackageFile = await existsSync( | ||
join(dpgContext.generationPathDetail?.metadataDir ?? "", "package.json") | ||
); | ||
const shouldGenerateMetadata = |
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.
Here is the emitter logic to detect if we need to generate metadata.
const hasTestFolder = await fsextra.pathExists( | ||
join(dpgContext.generationPathDetail?.metadataDir ?? "", "test") | ||
); | ||
const shouldGenerateTest = |
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.
Here is the emitter logic to detect if we need to generate test files.
@@ -718,6 +719,8 @@ function deserializeResponseValue( | |||
return required | |||
? `new Date(${restValue})` | |||
: `${restValue} !== undefined? new Date(${restValue}): undefined`; | |||
case "combined": | |||
return `${restValue} as any`; |
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.
Temp workaround for this issue #1921
error: !p.error ? undefined : p.error, | ||
})), | ||
created: new Date(result.body.result?.["created"]), | ||
data: result.body.result?.["data"] as any, |
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.
Workaround: #1921
fixes #1811 #1958
[Part 2] In this pr we would cover:
Please notice the change would only apply to typespec as input. For swagger if needed we could have another pr.