refactor: move out the loadTemplateConfig from Gigantic generator.js #1584
Conversation
Signed-off-by: ItsRoy69 <[email protected]>
ItsRoy69
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
|
""" WalkthroughThe changes refactor the template configuration loading logic in the generator by removing internal methods from the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes identified. Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
Hey @ItsRoy69 Have you run the test by |
|
@derberg can you please start the workflow? |
Signed-off-by: ItsRoy69 <[email protected]>
|
@ItsRoy69 go through these issues. |
|
here are some of the issues. |
Signed-off-by: ItsRoy69 <[email protected]>
|
@ItshMoh hey, updated the path accordingly and tested out , its passing now. |
|
@ItsRoy69 There are also some issues in dart client. fix it. The snapshots are obselete, generate new ones. |
|
@ItsRoy69 also run |
Signed-off-by: ItsRoy69 <[email protected]>
sure, will go through it , noticed many testings are failing in other files, looking into it |
|
@ItshMoh the integration issue is separate issue, I guess that can be handled in separate PR as its quite many changes required for that which is not related to template generator error |
@ItsRoy69 the integration issue is not the separate issue. it is due to the changes we made in this PR. So we need to fix it here. Otherwise how is it gonna be merged?. Look for it |
|
@ItsRoy69 modify the |
Signed-off-by: ItsRoy69 <[email protected]>
…into fix-template
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/generator/lib/templateConfig/loader.js (3)
7-10: Fix JSDoc annotation inconsistency.The function is marked as
@privatebut is exported in the module. Consider either removing the@privateannotation or adding appropriate public documentation./** * Loads the template configuration. - * @private + * @param {Object} context - The generator instance context (bound via .call()) */
41-45: Fix JSDoc annotation inconsistency.Same issue as the
loadTemplateConfigfunction - marked as@privatebut exported./** * Loads default values of parameters from template config. If value was already set as parameter it will not be - * overriden. - * @private + * overridden. + * @param {Object} context - The generator instance context (bound via .call()) */Note: Also fixed the typo "overriden" → "overridden".
46-58: Consider the context dependency design.The functions rely heavily on
thiscontext (accessingthis.templateDir,this.templateConfig,this.templateParams), which requires careful binding when called. While this maintains compatibility with the existing codebase during refactoring, consider whether a more explicit parameter-passing approach might be more maintainable long-term.For now, the current approach is acceptable given the refactoring context.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/generator/lib/generator.js(3 hunks)apps/generator/lib/templateConfig/loader.js(1 hunks)apps/generator/lib/templateConfig/validator.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/generator/lib/templateConfig/validator.js
- apps/generator/lib/generator.js
🔇 Additional comments (1)
apps/generator/lib/templateConfig/loader.js (1)
11-39: Excellent refactoring approach with proper error handling.The configuration loading logic is well-structured with appropriate fallback mechanisms:
- Primary attempt:
.ageneratorrc(YAML format)- Fallback:
package.jsonwithgeneratorproperty- Graceful error handling with debug logging
The refactoring successfully extracts this logic from the main generator file while maintaining the same behavior.
Signed-off-by: ItsRoy69 <[email protected]>
…into fix-template
Signed-off-by: ItsRoy69 <[email protected]>
|
|
Fixed the issues for which testing were failing after running |
Description
Related issue(s)
Resolves #1582
Summary by CodeRabbit
Refactor
Chores