Implemented option to include empty directory #180
Conversation
|
|
📝 WalkthroughWalkthroughThe pull request introduces modifications to the configuration and functionality of the project. A new property, Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
repomix.config.json (1)
10-11: Configuration changes look good, consider adding documentationThe new
includeEmptyDirectoriesoption is well-placed in the output section and its default value oftruemakes sense for backward compatibility.Consider updating the header text or README to document this new feature. Here's a suggested addition to the "Key Features" section:
Key Features: - Configurable ignore patterns - Custom header text support - Efficient file processing and packing +- Support for including empty directoriessrc/core/file/fileTreeGenerate.ts (1)
26-41: Consider adding path validation and conflict handlingWhile the implementation is functional, consider these improvements for robustness:
- Add validation for empty or invalid paths
- Handle path normalization (e.g., for different path separators)
- Add conflict detection when a path is treated as both file and directory
Example implementation:
const addPathToTree = (root: TreeNode, path: string, isDirectory: boolean): void => { + if (!path) { + throw new Error('Path cannot be empty'); + } + // Normalize path separators + const normalizedPath = path.replace(/[\\/]+/g, nodepath.sep); const parts = path.split(nodepath.sep); let currentNode = root; for (let i = 0; i < parts.length; i++) { const part = parts[i]; const isLastPart = i === parts.length - 1; let child = currentNode.children.find((c) => c.name === part); if (!child) { child = createTreeNode(part, !isLastPart || isDirectory); currentNode.children.push(child); + } else if (isLastPart && child.isDirectory !== isDirectory) { + throw new Error(`Path conflict: ${path} cannot be both file and directory`); } currentNode = child; } };src/core/output/outputGenerate.ts (1)
82-86: Consider architectural improvements for the empty directory featureWhile the implementation works, consider these architectural improvements:
- Move the empty directory logic to a separate function for better maintainability
- Consider caching the search results to avoid duplicate filesystem operations
- Add logging for debugging purposes, especially for empty directory handling
Example structure:
async function getEmptyDirectories(rootDir: string, config: RepomixConfigMerged): Promise<string[]> { if (!config.output.includeEmptyDirectories) { return []; } try { const searchResult = await searchFiles(rootDir, config); return searchResult.emptyDirPaths; } catch (error) { throw new RepomixError(`Failed to search for empty directories: ${error.message}`); } }src/core/packager.ts (1)
41-41: Consider updating progress callback messagesSince we're now handling empty directories, the progress messages should reflect this.
- progressCallback('Searching for files...'); + progressCallback('Searching for files and directories...');tests/core/packager.test.ts (1)
Line range hint
1-190: Consider adding specific test cases for empty directories.While the mock implementation supports empty directories, there are no explicit test cases verifying the behavior when
emptyDirPathscontains entries. Consider adding test cases to verify:
- Handling of empty directories when included
- Proper counting of empty directories in results
- Output generation with empty directories
Would you like me to help draft these additional test cases?
tests/core/file/fileSearch.test.ts (3)
60-65: Enhance mock implementation with stricter options checkingThe current mock implementation could be more robust by validating all relevant globby options.
Consider enhancing the mock implementation:
vi.mocked(globby).mockImplementation(async (_, options) => { if (options?.onlyDirectories) { + expect(options).toMatchObject({ + onlyDirectories: true, + onlyFiles: false, + dot: true, + followSymbolicLinks: false + }); return mockEmptyDirs; } + expect(options).toMatchObject({ + onlyDirectories: false, + onlyFiles: true, + dot: true, + followSymbolicLinks: false + }); return mockFilePaths; });
86-86: Improve error message clarityThe error message could be more descriptive to help developers understand the test failure.
-throw new Error('Should not search for directories when disabled'); +throw new Error('globby was called with onlyDirectories=true when includeEmptyDirectories is disabled');
49-49: Consider adding test cases for edge scenariosThe current test suite could benefit from additional coverage for:
- Empty directories with special characters or spaces
- Nested empty directories
- Error handling scenarios (e.g., permission issues)
Would you like me to help generate these additional test cases?
src/core/file/fileSearch.ts (2)
38-39: Consider elevating error logging level for directory read errorsErrors encountered while reading directories are currently logged at the
debuglevel. If a directory cannot be read due to permission issues or other errors, consider logging at a higher level (e.g.,warnorerror) to make users aware of potential issues during file search.Apply this change to adjust the logging level:
- logger.debug(`Error checking directory ${dir}:`, error); + logger.warn(`Error checking directory ${dir}:`, error);
87-100: Standardize error handling for directory and file searchesIn the file search, permission errors are explicitly caught and re-thrown as
PermissionError, providing clear feedback to the user. For consistency and better error reporting, consider applying similar error handling in the directory search when accessing directories.Update the
findEmptyDirectoriesfunction to handle permission errors:try { const entries = await fs.readdir(fullPath); const hasVisibleContents = entries.some((entry) => !entry.startsWith('.')); if (!hasVisibleContents) { // Existing logic... } } catch (error) { // Handle permission errors specifically + if (error.code === 'EPERM' || error.code === 'EACCES') { + throw new PermissionError( + `Permission denied while accessing directory: ${fullPath}`, + fullPath, + ); + } logger.debug(`Error checking directory ${dir}:`, error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
repomix.config.json(1 hunks)src/config/configSchema.ts(2 hunks)src/core/file/fileSearch.ts(2 hunks)src/core/file/fileTreeGenerate.ts(3 hunks)src/core/output/outputGenerate.ts(2 hunks)src/core/packager.ts(1 hunks)tests/core/file/fileSearch.test.ts(3 hunks)tests/core/packager.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Test (windows-latest, 22.x)
tests/core/file/fileSearch.test.ts
[failure] 72-72: tests/core/file/fileSearch.test.ts > fileSearch > getIgnoreFilePaths > should handle empty directories when enabled
AssertionError: expected [ 'empty-root', 'src/empty' ] to deeply equal [ 'src/empty', 'empty-root' ]
- Expected
-
Received
Array [
- "src/empty",
"empty-root",
- "src/empty",
]
❯ tests/core/file/fileSearch.test.ts:72:36
🔇 Additional comments (14)
repomix.config.json (1)
10-11: Verify the unrelated showLineNumbers change
The addition of showLineNumbers wasn't mentioned in the PR objectives. Please confirm if this is an intentional change related to the empty directories feature.
✅ Verification successful
Let me gather more specific information about the relationship between these changes.
Based on the evidence from the shell scripts, I can now generate the final response:
The showLineNumbers change is a valid configuration update
The showLineNumbers option is a well-established feature in the codebase:
- It's properly defined in the config schema with a default value of
false - Has dedicated test coverage in
fileProcess.test.tsfor line number functionality - Is consistently used across test fixtures and integration tests
- Setting it to
falsein the config file aligns with the default value in the schema
The change in repomix.config.json is simply making the default behavior explicit, likely for clarity, and is unrelated to the empty directories feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if showLineNumbers is related to includeEmptyDirectories
# Look for related changes in source files and tests
# Search for showLineNumbers usage
echo "Searching for showLineNumbers usage:"
rg -l 'showLineNumbers'
# Search for commits mentioning both features
echo -e "\nSearching for related commits:"
git log --grep="showLineNumbers\|empty.*director" --pretty=format:"%h %s"
Length of output: 851
Script:
#!/bin/bash
# Check the implementation and tests related to showLineNumbers
echo "Checking showLineNumbers implementation:"
ast-grep --pattern 'showLineNumbers' -f ts,js
# Check config schema definition
echo -e "\nChecking config schema definition:"
cat src/config/configSchema.ts
# Check test cases
echo -e "\nChecking test cases for showLineNumbers:"
rg -A 5 'showLineNumbers' tests/
Length of output: 11418
src/core/file/fileTreeGenerate.ts (4)
11-11: LGTM: Function signature properly updated
The addition of the optional emptyDirPaths parameter with a default empty array is well-typed and aligns with the feature requirements.
14-21: LGTM: Clean implementation of file and directory handling
The separate processing of files and empty directories provides clear separation of concerns and maintainable code structure.
71-74: LGTM: Clean update to generateTreeString
The function has been properly updated to support empty directories while maintaining existing behavior.
11-11: Verify updates to all callers
Since both generateFileTree and generateTreeString signatures have changed, we should verify that all callers are updated accordingly.
Also applies to: 71-71
✅ Verification successful
All callers are properly updated with the new parameter
The verification shows that both functions are called correctly with the new emptyDirPaths parameter:
generateFileTreeis called infileTreeGenerate.tswith both parameters:generateFileTree(files, emptyDirPaths)generateTreeStringis called inoutputGenerate.tswith both parameters:generateTreeString(allFilePaths, emptyDirPaths)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to these functions to ensure they're updated with the new parameter
echo "Searching for generateFileTree calls:"
rg "generateFileTree\(" --type ts -A 1
echo -e "\nSearching for generateTreeString calls:"
rg "generateTreeString\(" --type ts -A 1
Length of output: 575
src/config/configSchema.ts (2)
27-27: LGTM! The schema addition is well-structured.
The new includeEmptyDirectories option is correctly implemented as an optional boolean property within the output configuration object, maintaining consistency with other similar options.
27-27: Verify configuration usage across the codebase.
Let's ensure the new configuration option is properly handled throughout the codebase.
Also applies to: 58-58
✅ Verification successful
Let me gather more specific information about how empty directories are handled in the codebase.
The new configuration option is properly implemented and handled throughout the codebase
The codebase already has complete implementation for handling empty directories:
fileSearch.tscollects empty directories when enabledfileTreeGenerate.tsproperly includes empty directories in tree generationoutputGenerate.tscorrectly uses the configuration option- Test coverage exists for both enabled and disabled cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of includeEmptyDirectories configuration option
# Find files that might need to handle the new option
echo "Files potentially needing updates for the new option:"
rg -l "output\." --type ts
# Check for potential configuration parsing
echo -e "\nConfiguration parsing implementations:"
rg "repomixConfig\w+Schema\.parse" --type ts
# Look for file operations that might need to handle empty directories
echo -e "\nFile operation implementations that might need updates:"
rg "readdir|mkdir|rmdir|Directory" --type ts
Length of output: 11221
Script:
#!/bin/bash
# Check implementation details in key files
# Check fileSearch implementation
echo "=== File Search Implementation ==="
rg -A 10 "const searchFiles" src/core/file/fileSearch.ts
# Check if empty directories are already handled
echo -e "\n=== Empty Directory Handling ==="
rg "emptyDir|empty.*director" --type ts
# Check the file tree generation logic
echo -e "\n=== File Tree Generation ==="
rg -A 10 "addPathToTree" src/core/file/fileTreeGenerate.ts
# Check test coverage for empty directories
echo -e "\n=== Test Coverage for Directory Handling ==="
rg "empty.*director|directory.*empty" tests/ --type ts
Length of output: 3704
src/core/output/outputGenerate.ts (2)
6-6: LGTM: Import added for new functionality
The import is correctly placed and necessary for the empty directory feature.
86-86: Verify tree generation with empty directories
The parameter addition looks correct, but let's verify the integration with the tree generation logic.
✅ Verification successful
Let me gather more information about the implementation and test coverage.
Let me check the implementation of the file tree generation to verify how empty directories are handled.
Tree generation with empty directories is properly implemented
The implementation correctly handles empty directories:
- The
generateTreeStringfunction signature matches the usage, acceptingfilesandemptyDirPathsarrays - Empty directories are properly added to the tree structure via
addPathToTreewithisDirectory=true - The tree generation preserves directory markers with trailing slashes
- Test coverage confirms the feature works with both enabled and disabled empty directory configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the generateTreeString implementation handles empty directories correctly
# Check the implementation of generateTreeString
ast-grep --pattern 'function generateTreeString($_: $_[], $_: $_[]) {
$$$
}'
# Check for any test cases covering empty directory scenarios
rg -l "test.*empty.*director" --type=ts
Length of output: 157
Script:
#!/bin/bash
# Check the actual implementation of generateTreeString
rg "generateTreeString" -A 10 --type=ts
# Check the test file content for empty directory handling
cat tests/core/file/fileSearch.test.ts
Length of output: 10038
Script:
#!/bin/bash
# Check the implementation of file tree generation
cat src/core/file/fileTreeGenerate.ts
Length of output: 1937
src/core/packager.ts (1)
41-41: Verify empty directory handling in output generation
The generateOutput function needs to be checked to ensure it properly handles empty directories.
✅ Verification successful
Let me gather more information about the empty directory handling implementation.
Let me check one final piece about the file search implementation to ensure complete verification.
Empty directory handling is properly implemented
The codebase has comprehensive empty directory handling through:
- Optional configuration via
includeEmptyDirectoriesin the config schema - Implementation in
fileSearch.tsusingglobbywithonlyDirectoriesoption - Dedicated
findEmptyDirectoriesfunction - Integration in output generation that conditionally includes empty directories in the tree structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if generateOutput function handles empty directories
# Search for generateOutput implementation
ast-grep --pattern 'function generateOutput($$$) {
$$$
}'
# Search for any directory-related handling in the output generation
rg -A 5 'emptyDir|directory' src/core/output/
Length of output: 773
Script:
#!/bin/bash
# Check the generateTreeString implementation and config interface
ast-grep --pattern 'function generateTreeString($$$) {
$$$
}'
# Check the config interface for empty directory option
rg -A 3 'includeEmptyDirectories' src/
Length of output: 1168
Script:
#!/bin/bash
# Check the file search implementation for empty directories
rg -A 10 'const directories = await globby' src/core/file/fileSearch.ts
# Also check if there are any tests related to empty directory handling
rg -l 'empty.*director' test/
Length of output: 524
tests/core/packager.test.ts (1)
117-120: LGTM! Consistent mock implementation across test cases.
The mock maintains consistency with the new interface structure while preserving the security check test functionality.
tests/core/file/fileSearch.test.ts (1)
245-247: LGTM: Assertions properly handle the new FileSearchResult structure
The modifications to existing test cases correctly verify the new behavior by asserting both filePaths and emptyDirPaths.
Also applies to: 271-273
src/core/file/fileSearch.ts (2)
11-14:
Confirm the impact of the new FileSearchResult interface on existing code
The introduction of the FileSearchResult interface changes the return type of searchFiles, which previously returned string[]. Please verify that all usages of searchFiles in the codebase have been updated to handle the new return type to prevent any runtime errors.
Run the following script to find all invocations of searchFiles:
✅ Verification successful
The new FileSearchResult interface changes have been properly handled
The codebase analysis shows that all usages of searchFiles have been correctly updated to handle the new return type:
- In
src/core/packager.ts: Uses destructuring to getfilePathsfrom the result:const { filePaths } = await deps.searchFiles(rootDir, config) - In
src/core/output/outputGenerate.ts: Correctly accessesemptyDirPathsfrom the result - In
tests/core/packager.test.ts: Mock implementations and assertions are updated to use the new interface structure - In
tests/core/file/fileSearch.test.ts: All test cases properly verify bothfilePathsandemptyDirPathsproperties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all function calls to 'searchFiles' to ensure they handle the new return type.
# Use 'ast-grep' to find calls to 'searchFiles' excluding its own definition
ast-grep --pattern '$CALL(searchFiles($_, $_))' --lang ts --ignore src/core/file/fileSearch.ts
Length of output: 358
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all occurrences of 'searchFiles'
rg "searchFiles" -t ts -A 3 -B 3
Length of output: 8901
16-43: Verify handling of directories containing only hidden files
The findEmptyDirectories function considers directories containing only hidden files (e.g., files starting with .) as empty. Ensure that this behavior aligns with the intended functionality, as some users might expect such directories to be treated as non-empty.
Run the following script to list directories containing only hidden files:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
- Coverage 92.07% 91.63% -0.44%
==========================================
Files 35 35
Lines 1892 1949 +57
Branches 429 439 +10
==========================================
+ Hits 1742 1786 +44
- Misses 150 163 +13 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/core/output/outputGenerate.ts (1)
82-82: Remove commented code.The old implementation is commented out and should be removed since it's no longer needed and is tracked in version control.
- // const emptyDirPaths = config.output.includeEmptyDirectories ? (await searchFiles(rootDir, config)).emptyDirPaths : [];tests/core/file/fileSearch.test.ts (1)
245-247: LGTM! Consider adding test coverage for combined scenarios.The assertions for empty directory paths have been appropriately added to existing test cases. However, consider adding a test case that verifies the interaction between .gitignore rules and empty directory handling, such as:
- Empty directories within ignored paths
- Empty directories containing only ignored files
Example test case:
test('should handle empty directories with gitignore rules', async () => { const mockConfig = createMockConfig({ output: { includeEmptyDirectories: true, }, ignore: { useGitignore: true, customPatterns: [], }, }); const mockFilePaths = ['src/file1.js']; const mockEmptyDirs = ['src/empty', 'ignored/empty']; const mockGitignore = 'ignored/**'; vi.mocked(globby).mockImplementation(async (_, options) => { if (options?.onlyDirectories) { return mockEmptyDirs; } return mockFilePaths; }); vi.mocked(fs.readdir).mockResolvedValue([]); vi.mocked(fs.readFile).mockResolvedValue(mockGitignore); const result = await searchFiles('/mock/root', mockConfig); expect(result.filePaths).toEqual(mockFilePaths); expect(result.emptyDirPaths).toEqual(['src/empty']); // ignored/empty should be excluded });Also applies to: 271-273
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/core/output/outputGenerate.ts(2 hunks)tests/core/file/fileSearch.test.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/core/output/outputGenerate.ts
[warning] 85-93: src/core/output/outputGenerate.ts#L85-L93
Added lines #L85 - L93 were not covered by tests
🔇 Additional comments (4)
src/core/output/outputGenerate.ts (3)
6-6: LGTM!
The import of searchFiles is correctly added and follows project conventions.
96-96: Verify function signature updates.
The generateTreeString function signature has been updated to accept empty directory paths. Let's verify that all callers have been updated accordingly.
✅ Verification successful
Function signature is consistently used across the codebase
The verification shows that:
- The
generateTreeStringfunction is defined insrc/core/file/fileTreeGenerate.tswith the signature(files: string[], emptyDirPaths: string[] = []) - There is only one call site in
src/core/output/outputGenerate.tswhich correctly passes both parameters - No other usages were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to generateTreeString
ast-grep --pattern 'generateTreeString($_)'
# Look for the function definition to verify signature
ast-grep --pattern 'function generateTreeString($_) {
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Search for all calls to generateTreeString with ripgrep
rg "generateTreeString" -A 2
# Search for function definition
rg "function generateTreeString" -A 2
Length of output: 861
83-93: Add test coverage and improve error handling.
The error handling block is currently not covered by tests. Additionally, the error type assertion could be more specific.
- Consider using a more specific error type:
- if (error instanceof Error) {
+ if (error instanceof RepomixError) {
throw new RepomixError(`Failed to search for empty directories: ${error.message}`);
+ } else if (error instanceof Error) {
+ throw new RepomixError(`Unexpected error while searching empty directories: ${error.message}`);
}- Add test coverage for error scenarios:
// Test error handling
test('buildOutputGeneratorContext handles searchFiles error', async () => {
const config = { output: { includeEmptyDirectories: true } };
// Mock searchFiles to throw an error
jest.spyOn(fileSearch, 'searchFiles').mockRejectedValue(new Error('Mock error'));
await expect(buildOutputGeneratorContext(rootDir, config, [], [])).rejects
.toThrow('Failed to search for empty directories: Mock error');
});Let's verify the current test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-93: src/core/output/outputGenerate.ts#L85-L93
Added lines #L85 - L93 were not covered by tests
tests/core/file/fileSearch.test.ts (1)
50-96: LGTM! Well-structured test cases for empty directory handling.
The test cases thoroughly cover both scenarios (enabled/disabled) with proper mocking and assertions. The Windows compatibility issue has been addressed by sorting arrays before comparison.
iNerdStack
changed the title
|
@iNerdStack The implementation looks perfect to me - everything works great, and I appreciate you adding the test cases as well. I'll go ahead and merge this. Thanks again! I'll handle the release tomorrow. |
|
@yamadashy Thank you |
Added the option to include empty directory, also with the config flag
output.includeEmptyDirectoriesI also updated test cases to cover existing changes and added new test for checking flag option
Checklist
npm run testnpm run lintRelated: #161