Refactor searchInPairs and Enhance rtrimLines Functions#122
Refactor searchInPairs and Enhance rtrimLines Functions#122yamadashy merged 6 commits intoyamadashy:mainfrom
searchInPairs and Enhance rtrimLines Functions#122Conversation
WalkthroughThe pull request introduces significant modifications to the 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 (
|
![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
🧹 Outside diff range and nitpick comments (1)
src/core/file/fileManipulate.ts (1)
Line range hint
76-93:CompositeManipulatorusage without its implementation will cause runtime errors.The
CompositeManipulatorclass is still being used for handling.vueand.sveltefiles, but its implementation has been removed according to the AI-generated summary. This will result in runtime errors when attempting to manipulate these file types.Please ensure that the
CompositeManipulatorclass is reintroduced or refactor the manipulators for.vueand.sveltefiles to avoid using the missing class.Consider refactoring as follows:
-'.vue': new CompositeManipulator( - new StripCommentsManipulator('html'), - new StripCommentsManipulator('css'), - new StripCommentsManipulator('javascript'), -), -'.svelte': new CompositeManipulator( - new StripCommentsManipulator('html'), - new StripCommentsManipulator('css'), - new StripCommentsManipulator('javascript'), -), +'.vue': new StripCommentsManipulator('html'), +'.svelte': new StripCommentsManipulator('html'),This change assumes that treating
.vueand.sveltefiles as HTML for comment stripping purposes is acceptable. Otherwise, consider implementing appropriate manipulators for these file types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/core/file/fileManipulate.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/core/file/fileManipulate.ts (3)
109-109: Review the change in behavior for unsupported file types ingetFileManipulator.With the updated
getFileManipulator, the function now potentially returns a manipulator for file extensions that were previously unsupported (returningnull). This change may unintentionally alter how files of unknown types are handled elsewhere in the application.Please confirm that this change in behavior is intentional and does not introduce issues where unsupported file types are now being processed incorrectly.
48-49: Ensure hash comments within strings are not incorrectly removed.The regex for removing hash comments uses a negative lookbehind to avoid removing hash symbols within strings. However, please verify that it does not inadvertently remove hash symbols that are part of strings, especially in cases with escaped quotes or multi-line strings.
To check for potential issues, you can run the following script:
#!/bin/bash # Description: Find hash symbols inside string literals that may be incorrectly affected. # Expected: No results mean hash symbols within strings are preserved. fd --extension py --exec ast-grep --lang python --pattern $'"$_"\n#\n$_' --pattern $'\'$_\'\n#\n$_'
43-44: Verify that the regex for removing docstrings handles all cases accurately.The new regex simplifies docstring removal, but please ensure it correctly handles all variations of docstrings, including single-line docstrings, multi-line docstrings, docstrings with varying indentation, and edge cases where triple quotes might appear in strings or comments.
To confirm the regex handles all cases, you can run the following script:
|
@Mefisto04 However, it seems that the lint checks and tests are currently failing. Could you please take a look and address the following:
Thanks again for your contributions to improving Repopack's performance. |
Mefisto04
changed the title
searchInPairs and Enhance rtrimLines Functions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
- Coverage 76.57% 76.40% -0.18%
==========================================
Files 36 36
Lines 1665 1653 -12
Branches 269 266 -3
==========================================
- Hits 1275 1263 -12
Misses 390 390 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/core/file/fileManipulate.ts (3)
96-96: SimplifiedsearchInPairsfunctionThe simplification of
searchInPairsusingArray.some()is a good improvement. It enhances readability without changing the functionality.Consider refactoring the entire
removeHashCommentsmethod to improve its overall complexity and readability. One approach could be to use regex with negative lookahead to match hash comments that are not within string literals. For example:removeHashComments(content: string): string { return content.replace(/^([^"']*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'))*[^"']*#.*$/gm, '$1'); }This regex-based approach could potentially simplify the method significantly while maintaining the same functionality. However, ensure to thoroughly test any changes to maintain correctness.
103-103: Improved quote matching inremoveHashCommentsThe updated regex pattern for matching quotes is more comprehensive, correctly handling single, double, and triple quotes while avoiding escaped quotes. This is a good improvement in functionality.
Consider adding a comment explaining the regex pattern to improve maintainability. For example:
// Match single (', "), double (""", ''') quotes, excluding escaped quotes const openingQuote = content.slice(prevQuote + 1).search(/(?<!\\)(?:"|'|'''|""")/g) + prevQuote + 1;
Line range hint
126-143: Improved newline handling inremoveHashCommentsThe updated logic for handling newlines after hash comments is a good improvement. It correctly preserves newlines for comments inside strings while removing them for actual comments.
While the current implementation is correct, it's still quite complex. Consider simplifying this logic further. One approach could be to use regex with capture groups to handle both comments and string contents in a single pass. For example:
removeHashComments(content: string): string { return content.replace( /(["'])(?:(?=(\\?))\2.)*?\1|#.*?(?=\n|$)/g, (match, quote) => quote ? match : '' ); }This regex-based approach matches both string literals and comments, preserving the former and removing the latter. It could potentially simplify the method significantly while maintaining the same functionality. However, ensure to thoroughly test any changes to maintain correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/core/file/fileManipulate.ts (4 hunks)
🧰 Additional context used
🪛 Biome
src/core/file/fileManipulate.ts
[error] 106-106: This variable implicitly has the any type.
Variable declarations without type annotation and initialization implicitly have the any type. Declare a type or initialize the variable with some value.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (2)
src/core/file/fileManipulate.ts (2)
9-13: Improved implementation ofrtrimLinesThe refactoring of
rtrimLinesis a good improvement. The new implementation is more readable and potentially more efficient. It correctly trims trailing whitespace from each line while maintaining the same function signature.
Line range hint
1-214: Address unresolved issue with default manipulator for unknown extensionsWhile the changes to the
PythonManipulatorclass are good improvements, there's an unresolved issue from a previous review that should be addressed. ThegetFileManipulatorfunction still defaults to returningnullfor unknown file extensions, which might not be the most robust approach.Consider implementing a more graceful handling of unknown file extensions. Here's a suggestion based on the previous review comment:
export const getFileManipulator = (filePath: string): FileManipulator | null => { const ext = path.extname(filePath); if (manipulators[ext]) { return manipulators[ext]; } else if (['.js', '.jsx', '.ts', '.tsx'].includes(ext)) { return new StripCommentsManipulator('javascript'); } else { // Return a default manipulator that doesn't modify the content return new BaseManipulator(); } };This approach ensures that a valid
FileManipulatoris always returned, with a fallback to a non-modifying manipulator for truly unknown file types. This change would improve the robustness of the file manipulation system.🧰 Tools
🪛 Biome
[error] 106-106: This variable implicitly has the any type.
Variable declarations without type annotation and initialization implicitly have the any type. Declare a type or initialize the variable with some value.
(lint/suspicious/noImplicitAnyLet)
|
|
|
I see that secretlint is throwing an error. |
|
@yamadashy yes, i am using windows. |
|
I've committed the lint fixes to your branch. Let me know if you encounter any further issues. |
|
Hi, @Mefisto04 The implementation is perfect, thank you so much! I'll merge this. Thank you for your contribution! |
2 participants
This pull request includes the following changes:
Refactored
searchInPairs:hashIndexfalls within string literal pairs. This enhances code readability and maintainability.Enhanced
rtrimLinesFunction:String.prototype.trimEnd()for trimming trailing whitespace from each line, improving performance and efficiency.These changes aim to streamline the code while maintaining the existing functionality and ensuring that all tests pass successfully.
Summary by CodeRabbit
New Features
Refactor
CompositeManipulatorclass while restructuring manipulator instantiation and access.