feat: add generateOnly filtering on final paths and new test template #1773
Conversation
|
WalkthroughThis PR introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
Kartikayy007
changed the title
|
Kartikayy007
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/scripts/mailchimp/package-lock.jsonis excluded by!**/package-lock.jsonapps/generator/test/test-templates/react-template/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/generator/cli.js(4 hunks)apps/generator/lib/generator.js(11 hunks)apps/generator/lib/logMessages.js(2 hunks)apps/generator/lib/renderer/react.js(4 hunks)apps/generator/lib/utils.js(2 hunks)apps/generator/test/generator.test.js(4 hunks)apps/generator/test/integration.test.js(1 hunks)apps/generator/test/renderer.test.js(3 hunks)apps/generator/test/utils.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/generator/lib/generator.js (1)
apps/generator/lib/renderer/react.js (1)
written(86-93)
apps/generator/test/renderer.test.js (2)
apps/generator/lib/renderer/react.js (2)
written(86-93)content(70-70)apps/generator/lib/utils.js (1)
util(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-latest
🔇 Additional comments (35)
apps/generator/lib/logMessages.js (2)
44-50: LGTM! New log message helpers are consistent with existing patterns.The new
skipGenerateOnlyandgenerateOnlyNoMatchesfunctions follow the established conventions in this file.
80-82: LGTM! Exports updated correctly.The new message helpers are properly exported alongside existing functions.
apps/generator/lib/utils.js (5)
10-10: LGTM! Minimatch dependency added for glob pattern matching.
62-73: LGTM! Clean implementation of overwrite skip logic.The function correctly checks file existence first and only applies glob matching when needed.
86-102: LGTM! Well-structured file writing with filtering.The function properly:
- Calculates the relative path for glob matching
- Applies generateOnly filter first
- Then checks noOverwriteGlobs for existing files
- Returns a boolean indicating success
114-130: LGTM! Copy filtering mirrors write filtering correctly.The implementation is consistent with
writeFileWithFilteringand correctly usestargetPathfor the relative path calculation.
42-60: Verify negation-only pattern behavior is documented.The function requires at least one positive pattern for any file to be allowed; providing only negation patterns like
['!test*.js']will exclude nothing becauseallowednever becomestrue. If this whitelist semantic is intentional, ensure it's clearly documented in code comments or external documentation to prevent user confusion.apps/generator/test/utils.test.js (4)
129-144: LGTM! Test setup is well-structured.The test suite properly mocks dependencies and clears them after each test.
145-214: LGTM! Comprehensive generateOnly filtering tests.Good coverage of:
- Empty/unset generateOnly (allows all)
- Single and multiple pattern matching
- Negated patterns
- Complex glob patterns
- Proper logging verification
216-247: LGTM! noOverwriteGlobs tests cover key scenarios.Tests correctly verify:
- Skipping existing files that match
- Allowing new files even when matching noOverwriteGlobs
- Overwriting existing files that don't match
282-290: LGTM! File options passthrough test.Verifies that file mode options are correctly forwarded to the underlying writeFile call.
apps/generator/test/renderer.test.js (3)
2-6: LGTM! Mock updated to use the new filtering utility.The mock correctly simulates
writeFileWithFilteringreturningtruefor successful writes.
41-44: LGTM! Test correctly verifies single content writing.The test validates that
saveRenderedReactContentreturns the count of written files (1 for single content).
62-65: LGTM! Test correctly verifies multiple content writing.The test validates aggregation of written file counts (2 for two contents).
apps/generator/test/integration.test.js (3)
157-176: LGTM! Integration test for exact file matching.The test correctly verifies that only
package.jsonis generated when specified ingenerateOnly, andtest-file.mdis excluded.
178-197: LGTM! Integration test for glob pattern matching.Good coverage of glob-based filtering with
*.jsonpattern.
199-223: LGTM! Integration test for combined generateOnly and noOverwriteGlobs.The test correctly validates that:
- Pre-existing
package.jsoncontent is preserved (noOverwriteGlobs)test-file.mdis not generated (generateOnly filters it out)apps/generator/test/generator.test.js (3)
25-25: LGTM! Default value test for generateOnly.Verifies that
generateOnlydefaults to an empty array when not provided.
38-38: LGTM! Constructor test for generateOnly option.Verifies that
generateOnlyis correctly set from constructor options.Also applies to: 52-52
112-134: LGTM! Tests for warnIfNoGenerateOnlyMatches method.Good coverage of warning behavior:
- Warns when
generateOnlyis set but no files were generated- Does not warn when files were successfully generated
apps/generator/lib/renderer/react.js (4)
5-7: LGTM! Import updated for new filtering utility.
86-95: LGTM! Properly delegates to filtering utility.The implementation correctly:
- Uses
writeFileWithFilteringwith all necessary parameters- Returns 1 for successful writes, 0 for skipped files
109-113: LGTM! Correct aggregation of write results.The
Promise.allwithreducecorrectly accumulates the count of successfully written files. The(val || 0)handles any potential undefined/falsy values defensively.
66-66: Verify that all callers ofsaveContentToFileexplicitly passtargetDir.The default value
targetDir = process.cwd()may not match the actual generator target directory if callers do not explicitly provide this parameter. Review all invocations ingenerator.jsto ensure they pass the correcttargetDirvalue rather than relying on the default.apps/generator/cli.js (4)
22-22: LGTM! Consistent accumulator pattern.The
generateOnlyarray follows the same pattern asnoOverwriteGlobs, providing a clean way to accumulate multiple CLI arguments.
37-37: LGTM! Parser follows established pattern.The parser implementation is consistent with
noOverwriteParserand correctly accumulates glob patterns into the array.
92-92: LGTM! Well-documented CLI option.The option declaration is clear, properly documented, and follows Commander.js conventions. The description effectively communicates the filtering behavior to users.
160-160: Verify negation pattern support works as expected.The integration of
generateOnlylooks correct and follows the established pattern for passing options to the Generator. However, since the PR description mentions "supporting negated globs", please verify that negation patterns (e.g.,!*.test.js) work correctly in practice.apps/generator/lib/generator.js (7)
26-28: LGTM! Necessary imports for filtering functionality.The new filtering-aware write and copy utilities are properly imported alongside their non-filtering counterparts.
51-51: LGTM! Good optimization with Set conversion.Converting
GENERATOR_OPTIONSto a Set improves lookup performance from O(n) to O(1), andgenerateOnlyis correctly included in the valid options list.
83-83: LGTM! Well-documented constructor additions.The
generateOnlyparameter is properly documented, integrated into the constructor signature with appropriate defaults, and initialized as a public property following the same pattern asnoOverwriteGlobs.Also applies to: 97-97, 115-116
133-134: LGTM! Well-integrated counter and validation logic.The file counter tracking is properly initialized, reset at the start of each generation, and used to provide feedback via the warning method. The validation correctly uses the Set's
has()method for option checking.Also applies to: 163-163, 202-202, 210-210
889-896: LGTM! Filtering properly integrated into rendering.Both React and non-React rendering paths correctly apply filtering and track generated files. The different counting approaches (count vs. boolean) appropriately reflect that React templates can generate multiple files from a single render.
963-969: LGTM! Copy operations now respect filtering.Non-renderable files are now properly filtered using
copyFileWithFiltering, and the counter is correctly incremented when files are actually copied. This ensures consistent filtering behavior across all file generation operations.
1039-1049: LGTM! Helpful user feedback when patterns don't match.The warning method provides valuable feedback when
generateOnlypatterns are specified but no files are generated. The guard foroutput === 'fs'is correct, as string output mode doesn't involve file generation.
| it('writes file that matches generateOnly and does not match noOverwriteGlobs', async () => { | ||
| const jsonFilePath = path.join(targetDir, 'tsconfig.json'); | ||
| utils.exists = jest.fn().mockResolvedValue(true); | ||
|
|
||
| const result = await utils.writeFileWithFiltering(jsonFilePath, testContent, {}, targetDir, ['package.json'], ['*.json']); | ||
|
|
||
| expect(result).toBe(true); | ||
| expect(utils.writeFile).toHaveBeenCalledWith(jsonFilePath, testContent, {}); | ||
| }); |
There was a problem hiding this comment.
Test expectation appears incorrect.
The test expects tsconfig.json to be written when generateOnly: ['package.json'], but tsconfig.json doesn't match the pattern package.json. This should return false and skip writing.
Either the test expectation is wrong, or the generateOnly pattern should be ['*.json'] to match tsconfig.json.
- it('writes file that matches generateOnly and does not match noOverwriteGlobs', async () => {
- const jsonFilePath = path.join(targetDir, 'tsconfig.json');
+ it('writes file that matches generateOnly and does not match noOverwriteGlobs', async () => {
+ const jsonFilePath = path.join(targetDir, 'config.json');
utils.exists = jest.fn().mockResolvedValue(true);
- const result = await utils.writeFileWithFiltering(jsonFilePath, testContent, {}, targetDir, ['package.json'], ['*.json']);
+ const result = await utils.writeFileWithFiltering(jsonFilePath, testContent, {}, targetDir, ['*.json'], ['package.json']);
- expect(result).toBe(true);
- expect(utils.writeFile).toHaveBeenCalledWith(jsonFilePath, testContent, {});
+ expect(result).toBe(true);
+ expect(utils.writeFile).toHaveBeenCalledWith(jsonFilePath, testContent, {});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('writes file that matches generateOnly and does not match noOverwriteGlobs', async () => { | |
| const jsonFilePath = path.join(targetDir, 'tsconfig.json'); | |
| utils.exists = jest.fn().mockResolvedValue(true); | |
| const result = await utils.writeFileWithFiltering(jsonFilePath, testContent, {}, targetDir, ['package.json'], ['*.json']); | |
| expect(result).toBe(true); | |
| expect(utils.writeFile).toHaveBeenCalledWith(jsonFilePath, testContent, {}); | |
| }); | |
| it('writes file that matches generateOnly and does not match noOverwriteGlobs', async () => { | |
| const jsonFilePath = path.join(targetDir, 'config.json'); | |
| utils.exists = jest.fn().mockResolvedValue(true); | |
| const result = await utils.writeFileWithFiltering(jsonFilePath, testContent, {}, targetDir, ['*.json'], ['package.json']); | |
| expect(result).toBe(true); | |
| expect(utils.writeFile).toHaveBeenCalledWith(jsonFilePath, testContent, {}); | |
| }); |
🤖 Prompt for AI Agents
In apps/generator/test/utils.test.js around lines 271-279, the test currently
asserts that tsconfig.json is written even though generateOnly is
['package.json'] which doesn't match tsconfig.json; update the test to reflect
correct behavior by keeping generateOnly as ['package.json'] and changing
assertions to expect the function to return false and that utils.writeFile was
NOT called (i.e., expect(result).toBe(false);
expect(utils.writeFile).not.toHaveBeenCalled()); ensure the utils.exists mock
remains as-is so the skip behavior is exercised.
2 participants
Description
Add
generateOnlyfiltering on final output paths for all template writes/copies, with negated globs supported and copy paths filtered.Known
Hooks/conditional dirs remain ungated by
generateOnly(extra files likeasyncapi.yamlstill appear) needs to be workedTest are failing and needs to be fixed
Summary by CodeRabbit
Release Notes
--generate-onlyCLI option (short form-g) to selectively generate files matching glob patterns!) for excluding specific files✏️ Tip: You can customize this high-level summary in your review settings.