-
-
Notifications
You must be signed in to change notification settings - Fork 952
Make Unix/Linux path handling OS-independent for Windows compatibility #1173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Refactored path operations to use OS-independent methods: - Replaced hardcoded forward slash '/' string splits with pathlib properties (Path.stem, Path.name, Path.parent.name) - Used os.path.basename() for string-based path operations - Added UTF-8 encoding to YAML file operations for Windows compatibility - Added clarifying comment to distinguish URL parsing from file path operations Tested on Windows with all functionality working: - Scan Engine: Module discovery and loading (117 modules) - API: File download headers and path handling - WebUI: Language and graph discovery Changes are backwards compatible with Unix/Linux systems. Fixes issue regarding Windows path handling
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced string-based path manipulations with platform-agnostic pathlib/os.path calls, added UTF-8 file encoding and a few docstrings/comments, and a minor whitespace change. No public interfaces or control flow changes; behavior preserved. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
nettacker/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
nettacker/core/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (1)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR successfully refactors path handling operations across 5 core files to use OS-independent methods, ensuring Nettacker works correctly on both Windows and Unix-like operating systems. The changes replace hardcoded forward slash string operations with proper pathlib properties and os.path functions, which automatically handle platform-specific path separators.
Key Changes:
- Replaced string
.split("/")operations with pathlib properties (.stem,.parent.name) for extracting file path components - Added UTF-8 encoding to YAML file operations for Windows compatibility
- Replaced string splitting with
os.path.basename()for filename extraction
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| nettacker/core/utils/common.py | Added clarifying comment to distinguish URL key delimiter from file path separator |
| nettacker/core/template.py | Added UTF-8 encoding parameter when opening YAML module files to handle non-ASCII characters on Windows |
| nettacker/core/messages.py | Replaced string splitting with language.stem to extract language names from file paths |
| nettacker/core/arg_parser.py | Replaced string splitting with pathlib properties (.stem, .parent.name) in three locations: graph discovery, language loading, and module loading |
| nettacker/api/engine.py | Replaced string splitting with os.path.basename() for extracting filename in file download headers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @subhash-0000. * #1173 (comment) The following files were modified: * `nettacker/api/engine.py` * `nettacker/core/arg_parser.py` * `nettacker/core/messages.py` * `nettacker/core/template.py` * `nettacker/core/utils/common.py`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi maintainers This PR fixes Windows path compatibility issues by replacing Unix-specific path operations with OS-independent methods ( Changes:
The CI workflow is awaiting approval to run. Could a maintainer please approve the workflow? Thanks! Fixes #1039 |
|
Hi @subhash-0000, I tested PR #1173 locally on macOS. The path-handling changes work as expected, module resolution and the updated directory structure behave correctly. The only failures I encountered were due to Python 3.12 removing To address this and make the test suite compatible with Python 3.12+, I opened a separate test-only compatibility PR here: With that shim applied, all 214 tests pass locally, and PR #1173 runs cleanly. Happy to test anything else if needed! |
|
@deekshithaby Great to hear the path-handling changes work correctly on macOS and that module resolution is functioning as expected. I appreciate you opening #1180 to address the Python 3.12 SSL compatibility issue separately - that's very helpful! Looking forward to getting this merged once the maintainers approve the workflow and review the changes. Thanks again for your thorough testing! |
…ndling # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
Friendly ping @arkid15r @securestep9 This PR has been tested by @deekshithaby on macOS and is ready for review. The workflow is awaiting approval to run CI checks. Thanks for your time! |
This PR makes the Unix/Linux path handling OS-independent to ensure Nettacker works equally well on both Windows and Unix-like operating systems.
I've requested assignment for issue #933 and have completed the implementation with full testing on Windows.
Changes Made
Refactored path operations in 5 core files to use OS-independent methods:
Fixed graph discovery, language loading, and module loading in arg_parser
Fixed language discovery in messages module
Fixed file download headers in API engine
Added UTF-8 encoding to YAML file operations in template module for Windows compatibility
Added clarifying comment in common utils to distinguish URL parsing from file path operations
All changes replace hardcoded forward slash operations with pathlib properties and os.path functions.
Testing Performed
Tested comprehensively on Windows 10 with Python 3.10.0:
Scan Engine: All 117 modules discovered and loaded successfully
API: File download headers working correctly with Windows paths
WebUI: Language discovery (23 languages) and graph discovery (2 graphs) working
Module Loading: All YAML files parsed without encoding errors
Path Handling: Windows backslash paths working correctly
Database: SQLite connection and operations working
All changes are cross-platform and fully backwards compatible with Unix/Linux systems.
Fixes #933
Type of change
Bugfix (non-breaking change which fixes an issue)
Checklist
I've followed the contributing guidelines
I've run make pre-commit, it didn't generate any changes
I've run make test, all tests passed locally