Space was not supporting #3983
Space was not supporting #3983
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (1)
Ginger/Ginger/GeneralLib/General.cs (1)
Line range hint
680-689: Add unit tests for the new path handling functionality.The changes introduce complex path handling logic that should be thoroughly tested with various scenarios:
- Paths with different drive letters
- UNC paths
- Paths with spaces
- Mixed forward/backward slashes
- Environment variables
- Quoted paths
Would you like me to help create comprehensive unit tests for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Ginger/Ginger/GeneralLib/General.cs(1 hunks)
🔇 Additional comments (1)
Ginger/Ginger/GeneralLib/General.cs (1)
Line range hint 680-689: Verify the impact on existing path handling.
Let's check if there are any existing usages that might be affected by the changes.
| @@ -677,7 +677,7 @@ public static List<string> SplitWithPaths(string input) | |||
| { | |||
| return []; | |||
| } | |||
| var pattern = @"[^\s""']+|""([^""]*)""|'([^']*)'"; | |||
| var pattern = @"""[^""]*""|(?:C:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+"; | |||
There was a problem hiding this comment.
Update regex pattern to support all drive letters and path formats.
The current pattern only supports C: drive paths. This is too restrictive and may cause issues with paths using other drive letters.
Update the pattern to support:
-var pattern = @"""[^""]*""|(?:C:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+";
+var pattern = @"""[^""]*""|(?:[A-Za-z]:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+";📝 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.
| var pattern = @"""[^""]*""|(?:C:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+"; | |
| var pattern = @"""[^""]*""|(?:[A-Za-z]:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+"; |
Make path handling cross-platform compatible.
The current implementation is Windows-specific. Consider updating the pattern to handle:
- Forward slashes for cross-platform compatibility
- UNC paths (\server\share)
- Environment variables in paths
Consider using Path.GetFullPath() to normalize paths before pattern matching:
-var pattern = @"""[^""]*""|(?:C:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+";
+var pattern = @"""[^""]*""|(?:(?:[A-Za-z]:|\\\\[^\s\\]+\\[^\s\\]+|\.{1,2}|)(?:\\|/)[^\s]+)|[^\s]+";📝 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.
| var pattern = @"""[^""]*""|(?:C:\\[^ ]+(?: [^ ]+)*[^ ]+)|[^\s]+"; | |
| var pattern = @"""[^""]*""|(?:(?:[A-Za-z]:|\\\\[^\s\\]+\\[^\s\\]+|\.{1,2}|)(?:\\|/)[^\s]+)|[^\s]+"; |
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Bug Fixes
SplitWithPathsmethod for more accurate string processing.