Optimised _debug.py #166
Optimised _debug.py #166
Conversation
Now its better and more efficient, as well as less redundant, new function added to Checks.py which allows you to check the execution policy Signed-off-by: Shahm Najeeb <[email protected]>
DefinetlyNotAI
added
bug/High
WalkthroughThe pull request introduces changes to system debugging and checking mechanisms across two files. In Changes
Suggested labels
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (1)CODE/_debug.py (1)
Version comparison needs an upgrade! 📈 Using string comparison for versions isn't reliable. Let's use the +from packaging import version
@staticmethod
def compare_versions(local_version: str, remote_version: str):
+ try:
+ local_ver = version.parse(local_version)
+ remote_ver = version.parse(remote_version)
+
- if local_version == remote_version:
+ if local_ver == remote_ver:
log_debug.info(f"Version is up to date. Your Version: {local_version}")
- elif local_version > remote_version:
+ elif local_ver > remote_ver:
log_debug.warning(
"Version is ahead of the repository. "
f"Your Version: {local_version}, "
f"Repository Version: {remote_version}")
else:
log_debug.error(
"Version is behind the repository."
f"Your Version: {local_version}, Repository Version: {remote_version}")
+ except version.InvalidVersion as e:
+ log_debug.error(f"Invalid version format: {e}")Don't forget to add 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
CODE/_debug.py (1)
Line range hint
23-72: Let's make this code faster and cleaner! 🚀The file checking logic could use some optimization and better error handling.
@staticmethod def check_files(directory: str, required_files: list[str]): try: log_debug.debug(f"Checking directory: {directory}") if not os.path.exists(directory): log_debug.error(f"Directory {directory} does not exist.") + return False # Normalize paths once + def normalize_path(path: str) -> str: + return path.strip().replace("\\", "/").replace('"', '') # Use set for O(1) lookups instead of lists + actual_files = { + normalize_path(os.path.relpath(os.path.join(root, file), start=directory)) + for root, _, files in os.walk(directory) + for file in files + } + required_files_set = {normalize_path(f) for f in required_files} + missing_files = required_files_set - actual_files + extra_files = actual_files - required_files_set if missing_files: log_debug.error(f"Missing files: {', '.join(missing_files)}") + return False if extra_files: log_debug.warning(f"Extra files found: {', '.join(extra_files)}") log_debug.info("All required files are present.") + return True except Exception as e: log_debug.error(f"Unexpected error during file check: {e}") + return False
🧹 Nitpick comments (2)
CODE/_debug.py (2)
Line range hint
174-219: Let's make the debug function more robust! 💪The debug function could use better log file handling and organization.
def debug(): """ Executes system checks and logs results. """ # Clear Debug Log log_path = "../ACCESS/LOGS/DEBUG/DEBUG.LOG" + try: + if os.path.exists(log_path): + # Backup old log + backup_path = f"{log_path}.backup" + if os.path.exists(backup_path): + os.remove(backup_path) + os.rename(log_path, backup_path) - os.remove(log_path) + except OSError as e: + print(f"Warning: Could not manage log files: {e}") # Group related checks together + def run_system_checks(): + log_debug.info("=== System Checks ===") + log_debug.info("Admin privileges found" if Check.admin() else "Admin privileges not found") + log_debug.info("UAC enabled" if Check.uac() else "UAC disabled") + log_debug.info("Execution policy is unrestricted" if Check.execution_policy() else "Execution policy is not unrestricted") + def run_path_checks(): + log_debug.info("=== Path Information ===") + log_debug.info(f"Execution path: {psutil.__file__}") + log_debug.info(f"Global execution path: {sys.executable}") + log_debug.info(f"Local execution path: {sys.prefix}") + log_debug.info("Running in a virtual environment" if sys.prefix != sys.base_prefix else "Not running in a virtual environment") # Online Configuration Check config = HealthCheck.get_online_config() if config: HealthCheck.compare_versions(VERSION, config["System Settings"]["version"]) required_files = config["System Settings"].get("files", "").split(",") HealthCheck.check_files(".", required_files) + run_system_checks() + run_path_checks() python_version() for info in DebugCheck.cpu_info(): log_debug.info(info) log_debug.info(f"Debug: {DEBUG}")
Line range hint
1-219: Here's some big-picture advice! 🎯The switch from return values to logging is good, but we should consider a few architectural improvements:
- Consider using a logging configuration file instead of hardcoding log settings
- Add structured logging for better log parsing
- Consider adding metrics collection for monitoring system health
Example logging config file (
logging_config.yaml):version: 1 formatters: structured: format: '%(asctime)s - %(name)s - %(levelname)s - %(message)s' datefmt: '%Y-%m-%d %H:%M:%S' handlers: debug_file: class: logging.FileHandler formatter: structured filename: ../ACCESS/LOGS/DEBUG/DEBUG.log mode: w loggers: debug: level: DEBUG handlers: [debug_file] propagate: noThis would make the logging system more maintainable and configurable! 🔧
🛑 Comments failed to post (2)
CODE/logicytics/Checks.py (1)
25-32: 💡 Codebase verification
⚠️ Potential issueYo! We def need to make this code safer! 🛡️
The execution_policy check needs some serious upgrades to handle things that could go wrong. Here's what I found:
- The code's being used in
_debug.pyjust to show info, so if it fails, it's not gonna break anything major (phew! 😅)- We already have logging set up through
log_debugin_debug.py- Platform module is imported in
_debug.py, so we can use itBut here's the thing - we still need those safety checks! The current code could totally crash if something goes wrong, and that's not cool. The suggested changes in the review are spot on:
- Checking if we're on Windows (cuz PowerShell only works there, duh!)
- Adding a timeout (so it doesn't hang forever)
- Catching errors (cuz stuff can always go wrong)
- Using the logging that's already there
The review comment's suggestion is 💯 and we should definitely make those changes!
🔗 Analysis chain
Hey! We need to make this code more bulletproof! 🛡️
The new
execution_policymethod needs some love to handle errors better and work on different operating systems. Here's what's missing:@staticmethod def execution_policy() -> bool: + if platform.system() != "Windows": + return False try: result = subprocess.run( ["powershell", "-Command", "Get-ExecutionPolicy"], capture_output=True, text=True, + timeout=5 # prevent hanging ) + if result.returncode != 0: + return False return result.stdout.strip().lower() == "unrestricted" + except (subprocess.SubprocessError, OSError) as e: + log_debug.error(f"Failed to check execution policy: {e}") + return FalseAlso, let's verify if this method is used correctly:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if execution_policy is called with proper error handling rg -A 5 "execution_policy\(\)"Length of output: 660
Script:
#!/bin/bash # Let's check if platform is imported and if there's any error handling ast-grep --pattern 'import platform'Length of output: 109
Script:
#!/bin/bash # Check for any try-except blocks around execution_policy calls ast-grep --pattern 'try: $$$ execution_policy() $$$'Length of output: 60
Script:
#!/bin/bash # Check if logging is set up rg "import.*logging" CODE/Length of output: 122
Script:
#!/bin/bash # Check the full content of _debug.py to see how execution_policy is used cat CODE/_debug.pyLength of output: 7851
CODE/_debug.py (1)
161-173: 🛠️ Refactor suggestion
Let's make the Python version check more flexible! 🐍
The version check is too rigid with hardcoded values. We should make it configurable!
+RECOMMENDED_PYTHON_VERSION = (3, 11) +MINIMUM_PYTHON_VERSION = (3, 8) def python_version(): version = sys.version.split()[0] try: major, minor = map(int, version.split(".")[:2]) - if (major, minor) == (3, 11): + current_version = (major, minor) + if current_version < MINIMUM_PYTHON_VERSION: + log_debug.error(f"Python Version: {version} - Incompatible. Minimum required: {MINIMUM_PYTHON_VERSION[0]}.{MINIMUM_PYTHON_VERSION[1]}") + elif current_version == RECOMMENDED_PYTHON_VERSION: log_debug.info(f"Python Version: {version} - Perfect") - elif major == 3: + elif current_version >= MINIMUM_PYTHON_VERSION: log_debug.warning(f"Python Version: {version} - Recommended: {RECOMMENDED_PYTHON_VERSION[0]}.{RECOMMENDED_PYTHON_VERSION[1]}.x") else: log_debug.error(f"Python Version: {version} - Incompatible")📝 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.RECOMMENDED_PYTHON_VERSION = (3, 11) MINIMUM_PYTHON_VERSION = (3, 8) def python_version(): version = sys.version.split()[0] try: major, minor = map(int, version.split(".")[:2]) current_version = (major, minor) if current_version < MINIMUM_PYTHON_VERSION: log_debug.error(f"Python Version: {version} - Incompatible. Minimum required: {MINIMUM_PYTHON_VERSION[0]}.{MINIMUM_PYTHON_VERSION[1]}") elif current_version == RECOMMENDED_PYTHON_VERSION: log_debug.info(f"Python Version: {version} - Perfect") elif current_version >= MINIMUM_PYTHON_VERSION: log_debug.warning(f"Python Version: {version} - Recommended: {RECOMMENDED_PYTHON_VERSION[0]}.{RECOMMENDED_PYTHON_VERSION[1]}.x") else: log_debug.error(f"Python Version: {version} - Incompatible") except Exception as e: log_debug.error(f"Failed to parse Python Version: {e}")
DefinetlyNotAI
added
type/System
Pull Request Template
Prerequisites
--devflag, if required.PR Type
update
Description
This pull request includes several changes to the
CODE/_debug.pyandCODE/logicytics/Checks.pyfiles to improve logging and refactor code. The most significant changes involve modifying methods to use logging instead of returning status messages, adding new methods for checking Python version and execution policy, and removing the return of status messages from several methods.Logging improvements:
CODE/_debug.py: Modified theHealthCheck.check_files,HealthCheck.compare_versions, andDebugCheck.sys_internal_binariesmethods to use logging instead of returning status messages. [1] [2] [3] [4]CODE/_debug.py: Added a new methodpython_versionto log the Python version and its compatibility status.Code refactoring:
CODE/_debug.py: Removed the return of status messages from theHealthCheck.check_files,HealthCheck.compare_versions,DebugCheck.sys_internal_binaries, andDebugCheck.execution_policymethods. [1] [2] [3] [4]CODE/_debug.py: Simplified thedebugmethod by removing redundant logging calls and using the newpython_versionmethod. [1] [2]New method addition:
CODE/logicytics/Checks.py: Added a new methodCheck.execution_policyto check if the execution policy is unrestricted.These changes enhance the logging capabilities and simplify the code by removing unnecessary return values and redundant logging calls.
Now its better and more efficient, as well as less redundant, new function added to Checks.py which allows you to check the execution policy
Motivation and Context
Fixing huge bug with
--devand--debugwhere the logic was both broken and failedCredit
N/A
Issues Fixed
N/A
Summary by CodeRabbit
New Features
Bug Fixes