Replace hardcoded sys.platform checks with shared constants - #72
Conversation
… constants Use IS_WINDOWS/IS_MACOS from pqi_contants instead of inline sys.platform comparisons across server GUI and client modules for consistency and single source of truth.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralizes platform detection by replacing direct sys.platform comparisons with shared IS_WINDOWS / IS_MACOS constants, aiming to make platform-specific behavior consistent and easier to maintain.
Changes:
- Replace scattered
sys.platformplatform checks withIS_WINDOWS/IS_MACOS. - Update platform-specific GUI behavior (macOS Fusion palette, Windows-only grab action, default font selection).
- Route platform-specific setup selection through shared constants.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| PyQtInspect/pqi_server_gui.py | Switch macOS styling condition in main() to IS_MACOS. |
| PyQtInspect/pqi_gui/windows/attach_window.py | Gate Windows-only “grab” functionality via IS_WINDOWS constant. |
| PyQtInspect/pqi_gui/styles.py | Use shared constants to select platform default font. |
| PyQtInspect/pqi_gui/settings/ide_jumpers.py | Replace Windows platform check in IDE path lookup logic with IS_WINDOWS. |
| PyQtInspect/pqi_gui/platform_specific/init.py | Select correct Setup implementation using IS_WINDOWS / IS_MACOS. |
| PyQtInspect/pqi_gui/keyboard_hook_handler.py | Use IS_WINDOWS to select the keyboard hook implementation. |
| PyQtInspect/_pqi_bundle/pqi_monkey.py | Use IS_WINDOWS in argument quoting/unquoting helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if IS_WINDOWS: | ||
| defaultPath = _find_for_windows() | ||
| if defaultPath: | ||
| pqi_log.info(f'Found IDE path for {command_name} from commands on Windows: {defaultPath}') |
There was a problem hiding this comment.
This Windows-specific branch appears inside _find_for_linux(), which makes the function name misleading and harder to reason about. Consider either (a) moving the Windows branch into a platform-dispatch wrapper (e.g., _find_default_ide_path()), or (b) renaming/refactoring _find_for_linux() to reflect that it may run on multiple platforms.
| from PyQtInspect._pqi_bundle.pqi_contants import IS_WINDOWS, IS_MACOS | ||
| if IS_WINDOWS: # Windows | ||
| return 'Microsoft Yahei' | ||
| elif sys.platform == 'darwin': # macOS | ||
| elif IS_MACOS: # macOS | ||
| return 'Helvetica' | ||
| else: # Linux | ||
| return 'DejaVu Sans' |
There was a problem hiding this comment.
Importing IS_WINDOWS/IS_MACOS inside _get_default_font() adds repeated import overhead if this function is called frequently (e.g., during style recalculation). Prefer a module-level import for these constants (or caching the result) unless there is a concrete need to avoid import-time dependencies.
| from PyQtInspect._pqi_bundle.pqi_contants import IS_WINDOWS | ||
| if IS_WINDOWS: | ||
| class_ = WindowsKeyboardHookHandler | ||
| else: | ||
| class_ = DummyKeyboardHookHandler |
There was a problem hiding this comment.
Doing the IS_WINDOWS import inside __new__ obscures dependencies and makes the factory logic harder to scan. If there’s no import-cycle concern, prefer importing IS_WINDOWS at module scope to keep platform selection logic focused on behavior rather than imports.
Address PR review: move IS_WINDOWS/IS_MACOS imports from function scope to module top-level in styles.py, keyboard_hook_handler.py, and platform_specific/__init__.py.
Deploying pyqtinspect-open with
|
| Latest commit: |
3d1fa21
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cbd53781.pyqtinspect-open.pages.dev |
| Branch Preview URL: | https://dev-0-5-1-refactor-replace-h.pyqtinspect-open.pages.dev |
|
@copilot review again |
Summary
sys.platformcomparisons across 7 modules withIS_WINDOWS/IS_MACOSconstants frompqi_contants.py, establishing a single source of truth for platform detection._pqi_saved_modules.pycompatibility layer and unused packaging/launcher scripts (pack_exe.py,pack_exe_pyinstaller.py,run_server.py).Changed files
pqi_monkey.py— useIS_WINDOWSinremove_quotes_from_args/quote_argskeyboard_hook_handler.py— useIS_WINDOWSin factory__new__platform_specific/__init__.py— useIS_WINDOWS/IS_MACOSin_get_setup()ide_jumpers.py— useIS_WINDOWSin_find_default_ide_path_helperstyles.py— useIS_WINDOWS/IS_MACOSin_get_default_fontattach_window.py— useIS_WINDOWSinPidLineEditpqi_server_gui.py— useIS_MACOSinmain()Note
pqi_attach/still contains its own localIS_WINDOWS/IS_MACdefinitions — these can be unified in a follow-up PR.Test plan
🤖 Generated with Claude Code