generator icon indicating copy to clipboard operation
generator copied to clipboard

refactor: move out the `loadTemplateConfig` from `generator.js`

Open ItsRoy69 opened this issue 6 months ago • 25 comments

Description

  • Moved the loadTemplateConfig function to separate file for clean code.
  • Renamed and moved the validation function inside template config

Related issue(s) Resolves #1582

Summary by CodeRabbit

  • Refactor

    • Streamlined template configuration loading and default parameter handling in generators for improved maintainability without changing user experience.
  • Chores

    • Updated internal import paths for consistency and clarity; no impact on functionality or user experience.

ItsRoy69 avatar May 29 '25 06:05 ItsRoy69

⚠️ No Changeset found

Latest commit: 6e7077ff5d63e16e0fb58c05a7794588faea51b0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar May 29 '25 06:05 changeset-bot[bot]

"""

Walkthrough

The changes refactor the template configuration loading logic in the generator by removing internal methods from the Generator class and introducing new external asynchronous loader functions in a dedicated module. Associated import paths and test imports are updated to reflect this modularization, with no other logic or control flow changes.

Changes

Files/Paths Change Summary
apps/generator/lib/generator.js Removed internal private async methods loadTemplateConfig and loadDefaultValues; replaced with wrappers calling external implementations; updated imports accordingly.
apps/generator/lib/templateConfig/loader.js New module exporting async functions loadTemplateConfig and loadDefaultValues to load and process template configuration and default parameter values.
apps/generator/lib/templateConfig/validator.js Updated import paths for utility modules to use parent directory references.
apps/generator/test/generator.test.js Updated import paths for template config validator module in mocks and test setup.
apps/generator/test/templateConfigValidator.test.js Updated import path for validateTemplateConfig function.
apps/generator/lib/mocks/templateConfigValidator.js Updated mock import path for template config validator.

Assessment against linked issues

Objective Addressed Explanation
Move loadTemplateConfig out of generator.js into a new templateConfig/loader.js module (#1582)
Structure template config logic into templateConfig folder with validator.js and loader.js (#1582)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes identified.

Possibly related PRs

  • asyncapi/generator#1485: Introduced the original support for .ageneratorrc and updated the template config loading logic, which is now being modularized in this PR. """

📜 Recent review details

Configuration used: .coderabbit.yaml Review profile: CHILL Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe4253761ab805de09c7edaeeb7d74da6ec1ea9a and cc074811e9d03d583c34aef8c8842028ee4ec2da.

📒 Files selected for processing (2)
  • apps/generator/lib/generator.js (2 hunks)
  • apps/generator/lib/templateConfig/loader.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/generator/lib/generator.js
🔇 Additional comments (6)
apps/generator/lib/templateConfig/loader.js (6)

1-7: LGTM! Clean imports and appropriate constants.

The imports are well-organized and the CONFIG_FILENAME constant improves maintainability.


14-26: Excellent input validation implementation.

The function properly validates the input parameter with comprehensive checks for existence, type, and directory status. This addresses potential runtime errors early.


29-41: Well-structured fallback mechanism for configuration loading.

The function correctly attempts to load from .ageneratorrc first and provides appropriate error logging when it fails, allowing graceful fallback to package.json.


43-56: Robust package.json fallback with proper error handling.

The fallback logic correctly attempts to load configuration from package.json and handles both file reading and JSON parsing errors gracefully. The function appropriately returns an empty object when both configuration sources fail.


63-77: Efficient default value implementation using property getters.

The function elegantly uses Object.defineProperty with getters for lazy evaluation of default values, ensuring defaults are only applied to undefined parameters and computed on access rather than upfront.


79-82: Clean module exports following Node.js conventions.

The exports are clear and follow standard module patterns, making both functions available for external use.

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar May 29 '25 06:05 coderabbitai[bot]

Hey @ItsRoy69 Have you run the test by npm run test command?

ItshMoh avatar May 30 '25 05:05 ItshMoh

@derberg can you please start the workflow?

ItshMoh avatar May 30 '25 05:05 ItshMoh

@ItshMoh Yes, while I ran npm run test, @derberg I found issues related to other files like SendOperation.js, MessageExamples.js and AvailableOperations.js for which many cases are failing (not related to loadTemplateConfig functions), should I create a separate issue/pull request fixing these ?

ItsRoy69 avatar May 30 '25 05:05 ItsRoy69

@ItsRoy69 go through these issues. Change the path of the `templateConfigValidator' in generator.test.js and other files where it is occuring. The path has been changed so change its path where it has occured.

ItshMoh avatar May 30 '25 06:05 ItshMoh

gen_loadtemp_test_ffail_sc

here are some of the issues.
@ItsRoy69

ItshMoh avatar May 30 '25 06:05 ItshMoh

@ItshMoh hey, updated the path accordingly and tested out , its passing now.

ItsRoy69 avatar May 30 '25 06:05 ItsRoy69

@ItsRoy69 There are also some issues in dart client. fix it. These are not issues with the generator. It may be because of new changes. gen_loadTEmp_dart-sc

The snapshots are obselete, generate new ones.

ItshMoh avatar May 30 '25 06:05 ItshMoh

@ItsRoy69 also run npm run lint after the changes. if there is any error npm run lint:fix. And please also go through the https://github.com/asyncapi/generator/blob/master/CONTRIBUTING.md

ItshMoh avatar May 30 '25 06:05 ItshMoh

npm run test

sure, will go through it , noticed many testings are failing in other files, looking into it

ItsRoy69 avatar May 30 '25 06:05 ItsRoy69

@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 avatar May 30 '25 08:05 ItsRoy69

@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

ItshMoh avatar May 30 '25 11:05 ItshMoh

@ItsRoy69 modify the generator.test.js file. Where the tests are failing due to gen.loadTemplateConfig and gen.loadDefaultValues these are now not in the generator class.

ItshMoh avatar May 30 '25 11:05 ItshMoh

Fixed the issues for which testing were failing after running npm run test, npm run lint:fix, npm run lint

ItsRoy69 avatar May 30 '25 17:05 ItsRoy69

@ItsRoy69 remove the conflicts also.

ItshMoh avatar Jun 03 '25 06:06 ItshMoh

@derberg can you please look here mainly the decoulpling of the loadTemplateConfig from the generator.js

ItshMoh avatar Jun 03 '25 06:06 ItshMoh

@derberg can you run the workflow when you are free, and also if you can review the decoupling part.

ItshMoh avatar Jun 04 '25 09:06 ItshMoh

Hey @ItsRoy69 Can you please refactor the code as @derberg suggested?

ItshMoh avatar Jun 05 '25 14:06 ItshMoh

Yes, working on it

ItsRoy69 avatar Jun 06 '25 03:06 ItsRoy69

@ItshMoh refactored the code accordingly, tested npm run lint, npm run test and npm run lint:fix as well

ItsRoy69 avatar Jun 06 '25 11:06 ItsRoy69

hey @Adi-204 Can you please review this PR? It would be a great help.

ItshMoh avatar Jun 17 '25 04:06 ItshMoh

@ItshMoh I looked at PR and it is looking good, but I think @derberg is best person to guide with the changes done in app/generator.

Adi-204 avatar Jun 17 '25 07:06 Adi-204

@ItsRoy69 hey, do you plan to continue working on this one?

derberg avatar Jun 25 '25 13:06 derberg

closing, no response from contributor

@ItsRoy69 if you still want to work on it, feel free to open a new PR whenever ready

derberg avatar Jul 09 '25 08:07 derberg