Convert devcontainer postCreateCommand to Python with Plumbum and Pathlib#56
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: danelkay93 <24777308+danelkay93@users.noreply.github.com>
Co-authored-by: danelkay93 <24777308+danelkay93@users.noreply.github.com>
a974b30
into
claude/update-documentation-integration-011CULn7AGnkyHBdk8qWi4qx
There was a problem hiding this comment.
Pull Request Overview
This PR converts the devcontainer's post-create setup script from Bash to Python using Plumbum and Pathlib, addressing concerns about unsafe wildcard chmod operations that could mask missing files.
Key Changes:
- Python implementation with explicit file existence checks using Pathlib
- Enhanced safety through individual file processing with feedback for each operation
- Added
plumbumdependency to devcontainer for colored terminal output and command execution - Python cache patterns added to
.gitignore
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
.devcontainer/postCreateCommand.py |
New Python script implementing devcontainer setup with Pathlib for file operations and Plumbum for colored output and command execution |
.devcontainer/devcontainer.json |
Updated to invoke Python script instead of Bash script for post-create command |
.devcontainer/Dockerfile |
Added plumbum to Python dependencies |
.gitignore |
Added Python cache directories (__pycache__/, *.pyc, *.pyo) |
scripts/check-pyscript-version.sh |
New placeholder script for PyScript version checking (to be integrated with Husky) |
Comments suppressed due to low confidence (11)
.devcontainer/postCreateCommand.py:9
- Import of 'local' is not used.
from plumbum import local, colors
.devcontainer/postCreateCommand.py:87
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:91
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:102
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:110
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:117
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:127
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:137
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:170
- Except block directly handles BaseException.
except:
.devcontainer/postCreateCommand.py:87
- 'except' clause does nothing but pass and there is no explanatory comment.
except:
.devcontainer/postCreateCommand.py:91
- 'except' clause does nothing but pass and there is no explanatory comment.
except:
|
|
||
| import sys | ||
| from pathlib import Path | ||
| from plumbum import local, colors |
There was a problem hiding this comment.
The local import from plumbum is not used anywhere in the script. Consider removing it:
from plumbum import colors| from plumbum import local, colors | |
| from plumbum import colors |
| def command_exists(cmd: str) -> bool: | ||
| """Check if a command exists in PATH""" | ||
| try: | ||
| subprocess.run( | ||
| ["command", "-v", cmd], | ||
| shell=True, | ||
| executable="/bin/bash", | ||
| check=True, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL | ||
| ) | ||
| return True | ||
| except subprocess.CalledProcessError: | ||
| return False |
There was a problem hiding this comment.
The command_exists function has a critical bug. Using shell=True with a list of arguments doesn't work as intended. The command ["command", "-v", cmd] will be joined and executed differently than expected.
Instead, use one of these approaches:
- Use
shell=Falseand pass the arguments as a list:["bash", "-c", f"command -v {cmd}"] - Use
shell=Truewith a string:f"command -v {cmd}" - Use
shutil.which(cmd)which is a more Pythonic approach
Recommended fix:
import shutil
def command_exists(cmd: str) -> bool:
"""Check if a command exists in PATH"""
return shutil.which(cmd) is not NoneOr if you want to keep the subprocess approach:
def command_exists(cmd: str) -> bool:
"""Check if a command exists in PATH"""
try:
subprocess.run(
["bash", "-c", f"command -v {cmd}"],
check=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL
)
return True
except subprocess.CalledProcessError:
return False| except: | ||
| pass | ||
| try: | ||
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception: to avoid catching system exceptions like KeyboardInterrupt and SystemExit:
try:
git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]()
except Exception:
pass| except: | |
| pass | |
| try: | |
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | |
| except: | |
| except Exception: | |
| pass | |
| try: | |
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | |
| except Exception: |
| try: | ||
| git["fetch", "origin"](retcode=None, stderr=subprocess.DEVNULL) | ||
| print_success("PR refs fetched") | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception::
try:
git["fetch", "origin"](retcode=None, stderr=subprocess.DEVNULL)
print_success("PR refs fetched")
except Exception:
print_warning("Could not fetch PR refs (authentication may be needed)")| except: | |
| except Exception: |
| user_name = git["config", "user.name"]().strip() | ||
| if not user_name: | ||
| raise ValueError("Empty user.name") | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception::
try:
user_name = git["config", "user.name"]().strip()
if not user_name:
raise ValueError("Empty user.name")
except Exception:
print_warning("Git user.name not set. Set it with: git config user.name 'Your Name'")| except: | |
| except Exception: |
| try: | ||
| python_version = python3["--version"]().strip() | ||
| print_success(f"Python is available: {python_version}") | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception::
try:
python_version = python3["--version"]().strip()
print_success(f"Python is available: {python_version}")
except Exception:
print_success("Python is available")| except: | |
| except Exception: |
| try: | ||
| npm["run", "build"](stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) | ||
| print_success("Build verification passed") | ||
| except: |
There was a problem hiding this comment.
Bare except: clause is considered bad practice in Python. Replace with except Exception::
try:
npm["run", "build"](stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
print_success("Build verification passed")
except Exception:
print_warning("Build verification failed - you may need to run 'npm run build' manually")| except: | |
| except Exception: |
| except: | ||
| pass | ||
| try: | ||
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python as they catch all exceptions, including KeyboardInterrupt and SystemExit, which can make debugging difficult and prevent graceful shutdown.
Replace with except Exception: to catch only regular exceptions while allowing system exits and keyboard interrupts to propagate:
try:
git["config", "--add", "remote.origin.fetch", "+refs/pull/*/head:refs/remotes/origin/pr/*"]()
except Exception:
pass| except: | |
| pass | |
| try: | |
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | |
| except: | |
| except Exception: | |
| pass | |
| try: | |
| git["config", "--add", "remote.origin.fetch", "+refs/pull/*/merge:refs/remotes/origin/pr-merge/*"]() | |
| except Exception: |
| user_email = git["config", "user.email"]().strip() | ||
| if not user_email: | ||
| raise ValueError("Empty user.email") | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception::
try:
user_email = git["config", "user.email"]().strip()
if not user_email:
raise ValueError("Empty user.email")
except Exception:
print_warning("Git user.email not set. Set it with: git config user.email 'your.email@example.com'")| except: | |
| except Exception: |
| try: | ||
| gh["auth", "status"](stderr=subprocess.DEVNULL) | ||
| print_success("GitHub CLI is authenticated") | ||
| except: |
There was a problem hiding this comment.
Bare except: clauses are considered bad practice in Python. Replace with except Exception::
try:
gh["auth", "status"](stderr=subprocess.DEVNULL)
print_success("GitHub CLI is authenticated")
except Exception:
print_warning("GitHub CLI not authenticated. Run: gh auth login")| except: | |
| except Exception: |
Pull Request
Description
Converts
.devcontainer/postCreateCommand.shto Python implementation using Plumbum and Pathlib. Addresses sourcery-ai feedback about unsafe wildcard chmod operations that could mask missing files.Related Issues
Related to #39
Addresses review comment: #39 (comment)
Type of Change
Changes Made
Modified Files
.devcontainer/Dockerfile- Addedplumbumto Python dependencies.devcontainer/devcontainer.json- Changed postCreateCommand to use Python script.gitignore- Added Python cache directories (__pycache__/,*.pyc,*.pyo)New Files
.devcontainer/postCreateCommand.py- Python rewrite with Plumbum and PathlibKey Improvements
Before (Bash):
Silently fails on wildcards, potentially masking issues.
After (Python with Pathlib):
Testing
Testing Steps
py_compileTest Results
Build Status: ✅ Pass (syntax validation)
Breaking Changes
Breaking Changes: No
Existing bash script remains in place. Devcontainer configuration updated to use Python version.
Documentation
Checklist
Code Quality
Testing & Validation
Documentation
Review Readiness
Impact Assessment
Performance Impact: Neutral (runs once at container creation)
Bundle Size Impact: No change (Python dependency only affects devcontainer)
Backward Compatibility: Maintained
Deployment Notes
Requires:
Container rebuild required to apply changes.
Reviewer Notes
Review Focus:
|| truepatternsAdditional Context
For AI Agents:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.