Codegen 643 #646
Codegen 643 #646
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thx for your pull request, it is inspiring for the feature design of multiple output targets. It would be better if
// these declarations are implemented separately from cli.ts
// the `AbsolutePath` is based on the schema file path instead of the file system's
declare function resolveDependencies(entrySchema: code): Map<AbsolutePath, AbsolutePath[]>;
// similar to the `codegenReturnMore` in this PR, with some enhancement
declare function codegen(code: string): CodegenResult;
type CodegenResult = {
code: string;
elements: string[];
}
// helpers
declare function extractAndEraseImportClauses(code: string): string;
type AbsolutePath = string;
// so it can be more readable in the cli.ts
const dependencies = resolveDependencies(entrySource)
Object.entries(dependencies).forEach(([path, dependentPaths]) => {
// ...
})
|
|
OK, based on your suggestions, I will make optimizations |
If you have more suggestions, please continue to reply. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #646 +/- ##
===========================================
- Coverage 84.17% 83.89% -0.29%
===========================================
Files 120 121 +1
Lines 24357 24553 +196
Branches 2341 2355 +14
===========================================
+ Hits 20503 20599 +96
- Misses 3813 3913 +100
Partials 41 41
Continue to review full report in Codecov by Sentry.
|
| return [cur]; | ||
| } | ||
|
|
||
| const matched = schema.match(/.*import\s+"(.*)".*;/g); |
There was a problem hiding this comment.
I found that the import statement definition should exclude the "", even though, there is something unexpected when a clause that includes a pattern of import "";, e.g.
vector string_import <byte>; // e.g. import "string";
maybe a capturing group could be helpful here
There was a problem hiding this comment.
This is a detail I overlooked, it really has a big impact
| const _dependencies = resolveDependencies( | ||
| mRelativePath, | ||
| baseDir, | ||
| resolved | ||
| ); | ||
| dependencies.push(..._dependencies); |
There was a problem hiding this comment.
| const _dependencies = resolveDependencies( | |
| mRelativePath, | |
| baseDir, | |
| resolved | |
| ); | |
| dependencies.push(..._dependencies); | |
| dependencies.push(...resolveDependencies( | |
| mRelativePath, | |
| baseDir, | |
| resolved | |
| )); |
It's better to avoid using variables that start with an underline as they often imply that the variable is private and mutable in a scope.
There was a problem hiding this comment.
I will avoid this way of writing in the future
| return delImportLines.join("\n"); | ||
| } | ||
|
|
||
| function printOrWrite(resultMap: Map<string, ParseResult>, config: Config) { |
There was a problem hiding this comment.
It is suggested to divide the function based on distinct output targets to make it more testable.
There was a problem hiding this comment.
Yes, that's better，agree with you.
| export function loopCodegen(config: Config): Map<string, ParseResult> { | ||
| const result: Map<string, ParseResult> = new Map(); | ||
| const baseDir = path.dirname(config.schemaFile); | ||
| const relativePath = path.basename(config.schemaFile); |
There was a problem hiding this comment.
I found that it's a basename rather than a relativePath, which could be misleading in this context
| export function resolveDependencies( | ||
| importPath: RelativePath, | ||
| baseDir: string, | ||
| resolved: Set<RelativePath> | ||
| ): FileWithDependence[] { |
There was a problem hiding this comment.
When defining a function with similar parameters, like importPath, baseDir, and resolvedRelativePath, it is recommended to comment them
|
Hi, @coder-van, your PR is inspiring, and I plan to make some improvements to it based on your work |
As for the details of the code, I may need some time to adjust it with the team. Thank you for your careful review, it is very helpful to me |
|
Hi, @coder-van, I've created a PR at #648 for addressing the issue #643. Thanks for your contribution again. I hope that this #648 PR will better express the optimizations I mentioned earlier. |
|
I've read your code of PR #648, it's great, made me realize that in this project, higher code quality is needed, thank you. |
|
After seeing your code, I suddenly realized that I wrote it in a simple way to complete the task, and you wrote it in order to achieve a complete solution, higher goals, and output better results. I saw several points from your output. What I praise:
There may be a point that can be considered for optimization. Currently, after all files are read and the import statements are processed, the existence of dependent files is only checked when recursively building the path. Is it better to do it earlier? |
Exactly. We can detect the existence of the molecule file while building the dependency graph so that the process could exit earlier if any dependence is missed |
|
@coder-van Thx for your PR that inspired me to create the #648 as an improvement. However, after reviewing your PR, I noticed a few issues, such as the lack of circular checking and test cases. I apologize for being nitpicky and closing it |
Description
generate TypeScript code recursively from mol type source file. Add config option output, when output = 0 (default), print TypeScript code only, and when output = 1, write to files.
Fixes #643
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
yarn run test
Checklist: