-
Notifications
You must be signed in to change notification settings - Fork 182
chore: add type generation improvements for ts #1100
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
Conversation
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.
Pull Request Overview
Adds improvements to the TypeScript type generation in the Appwrite CLI, including single-file output support, optional property syntax, and import/formatting fixes.
- Introduce
--output
as a file path for single-file languages and validate directory vs. file output. - Switch optional model attributes from
| null
to the?
shorthand and update import statements. - Enhance templates with auto-generated comments and cleaner enum formatting.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
templates/cli/lib/type-generation/languages/typescript.js.twig | Update type rendering: optional ? , import type Models , add regen instructions. |
templates/cli/lib/commands/types.js.twig | Detect file vs. directory output, support single-file destinations, error on mismatch. |
Comments suppressed due to low confidence (2)
templates/cli/lib/commands/types.js.twig:69
- [nitpick] The alias
rawOutputPath
forrawOutputDirectory
may be confusing; consider renaming the function parameter torawOutputPath
directly or removing the extra alias for clarity.
const rawOutputPath = rawOutputDirectory;
templates/cli/lib/commands/types.js.twig:71
- Add unit tests for the new single-file output logic (e.g., when
isFileOutput
is true andmeta.isSingleFile()
), to ensure correct path resolution and error handling.
const isFileOutput = !!outputExt;
outputDirectory = path.dirname(rawOutputPath); | ||
singleFileDestination = rawOutputPath; | ||
} else { | ||
throw new Error("Output path must be a directory for languages that generate multiple files."); |
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 error message could include the invalid path value to aid debugging, e.g., Invalid output path: ${rawOutputPath}. Directory required for multi-file languages.
throw new Error("Output path must be a directory for languages that generate multiple files."); | |
throw new Error(`Invalid output path: ${rawOutputPath}. Output path must be a directory for languages that generate multiple files.`); |
Copilot uses AI. Check for mistakes.
<% for (const [index, element] of Object.entries(attribute.elements)) { -%> | ||
<%- toUpperSnakeCase(element) %> = "<%- element %>", | ||
<% const entries = Object.entries(attribute.elements); -%> | ||
<% for (let i = 0; i < entries.length; i++) { -%> |
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 too complex for me, sorry. Can you show me what this code looks like generated?
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 change is just to remove the comma at the end of enum class:
before:
export enum Gender {
MALE = "male",
FEMALE = "female",
NON_BINARY = "non-binary",
}
after:
export enum Gender {
MALE = "male",
FEMALE = "female",
NON_BINARY = "non-binary"
}
@@ -95,7 +110,7 @@ const typesCommand = actionRunner(async (rawOutputDirectory, {language}) => { | |||
getType: meta.getType | |||
}); | |||
|
|||
const destination = path.join(outputDirectory, meta.getFileName()); | |||
const destination = singleFileDestination || path.join(outputDirectory, meta.getFileName()); |
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.
Let's do basic QA, make sure this work in scenarios like:
- Folder name but with a dot (not file)
- file in nested non-existing 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.
What does this PR do?
?
instead of| null
Models
import errorTest Plan
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
yes