Refactor file operations to use pathlib #285
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors platform.py to use pathlib.Path for filesystem operations, updates safe_* helpers to accept Union[str, Path], adds a cached packages_dir property, adjusts IDF tooling path initialization to use PROJECT_CORE_DIR, and applies Path-based logic across tool installation, cleanup, and debug session checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Platform as Espressif32Platform
participant FS as FileSystem (pathlib.Path)
participant Version as VersionHelper
participant Installer as ToolInstaller
Caller->>Platform: initialize / configure_default_packages
Platform->>FS: resolve project/core and packages_dir (Path)
Platform->>Version: extract/compare tl-install versions (may parse URLs)
alt install or upgrade needed
Platform->>Installer: perform install/update (Path targets)
Installer->>FS: write/copy/remove files and dirs (Path helpers)
Platform->>FS: cleanup legacy/versioned dirs (rglob/iterdir)
else up-to-date
Platform-->>Caller: no install needed
end
Platform->>FS: validate debug flash image(s) with Path.is_file()
Platform-->>Caller: configuration complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
platform.py (7)
134-141: Handle broken symlinks in safe_remove_file.
unlink will be skipped for broken symlinks because is_file() is False. Consider unlinking symlinks regardless of target existence.Apply:
def safe_remove_file(path: Union[str, Path]) -> bool: """Safely remove a file with error handling using pathlib.""" path = Path(path) - if path.exists() and path.is_file(): - path.unlink() + # Remove regular files or symlinks (including broken ones) + if path.is_file() or path.is_symlink() or path.exists(): + path.unlink() logger.debug(f"File removed: {path}") return True
154-165: Optional: consider supporting nested matches (if needed).
Current impl only deletes direct children. If you ever need to clean subdirs, switch to rglob.- for item in base_path.iterdir(): + for item in base_path.rglob("*"): if item.is_dir() and fnmatch.fnmatch(item.name, pattern): shutil.rmtree(item) logger.debug(f"Directory removed: {item}")
168-175: Preserve file metadata during copies.
Use copy2 to retain timestamps/permissions; helps reproducibility.- shutil.copyfile(src, dst) + shutil.copy2(src, dst)
178-185: Preserve metadata when copying directories.
Leverage copy_function=shutil.copy2.- shutil.copytree(src, dst, dirs_exist_ok=True) + shutil.copytree(src, dst, dirs_exist_ok=True, copy_function=shutil.copy2)
198-204: Consider returning Path for packages_dir.
Uniform Path return simplifies call sites (you already cast to Path repeatedly). Not mandatory.- def packages_dir(self) -> str: + def packages_dir(self) -> Path: @@ - self._packages_dir = config.get("platformio", "packages_dir") + self._packages_dir = Path(config.get("platformio", "packages_dir")) return self._packages_dirNote: adjust usages that expect str (e.g., when serializing) via str(...).
374-376: Prefer logger.exception to keep traceback on cleanup failures.
Improves diagnosability without changing behavior.- except OSError as e: - logger.error(f"Error cleaning up versioned directories for {tool_name}: {e}") + except OSError: + logger.exception(f"Error cleaning up versioned directories for {tool_name}")
654-663: Prefer logger.exception when JSON parsing fails.
Keeps the stacktrace and the failing path context.- except (json.JSONDecodeError, KeyError) as e: - logger.error(f"Error reading mklittlefs package {e}") + except (json.JSONDecodeError, KeyError): + logger.exception("Error reading mklittlefs package metadata")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platform.py(12 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
platform.py
📚 Learning: 2025-07-29T11:35:26.182Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
Applied to files:
platform.py
🪛 Ruff (0.12.2)
platform.py
375-375: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
665-665: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/espidf-ulp)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
🔇 Additional comments (6)
platform.py (6)
37-38: Pathlib + typing imports: solid baseline for this refactor.
Consistent with the PR goal; no issues spotted.
99-101: IDF_TOOLS_PATH override matches project policy.
This aligns with the prior decision to always set IDF_TOOLS_PATH to PlatformIO’s core_dir.
144-151: LGTM: directory removal path is straightforward and safe behind the decorator.
899-899: Nice Path-based validation in debug session.
The switch to Path(...) for flash image checks is correct and clearer.
498-506: Build a proper file:// URI on all OSes.
Current f-string may produce an invalid URI on Windows (backslashes). Use as_uri().- tl_path = f"file://{Path(IDF_TOOLS_PATH) / 'tools' / tool_name}" + tl_path = (Path(IDF_TOOLS_PATH) / 'tools' / tool_name).resolve().as_uri()⛔ Skipped due to learnings
Learnt from: Jason2866 PR: pioarduino/platform-espressif32#263 File: platform.py:492-492 Timestamp: 2025-08-20T14:13:54.005Z Learning: In PlatformIO projects, `file://` is a special PlatformIO syntax for path references, not standard file URIs. The construction `f"file://{os.path.join(path, subpath)}"` is correct PlatformIO syntax and should not be replaced with pathlib.Path.as_uri().
321-325: pm.install receives full URL spec from platform.json
Allversionentries in platform.json are URLs (not bare semvers), so passingversiondirectly topm.install()is already unambiguous and correct.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
platform.py (3)
154-164: Targeted pattern deletion and safer iteration.
Walking the whole tree then deleting during traversal is brittle. Use a precomputed list with globbing.- for item in base_path.rglob("*"): - if item.is_dir() and fnmatch.fnmatch(item.name, pattern): - shutil.rmtree(item) - logger.debug(f"Directory removed: {item}") + matches = list(base_path.glob(f"**/{pattern}")) + for item in matches: + if item.is_dir(): + shutil.rmtree(item) + logger.debug(f"Directory removed: {item}")
221-223: Redundant Path wrapping.
packages_diralready returns a Path; simplify.- tl_install_path = Path(self.packages_dir) / tl_install_name - package_json_path = tl_install_path / "package.json" + tl_install_path = self.packages_dir / tl_install_name + package_json_path = tl_install_path / "package.json"
686-688: Add an inline comment to explain the package.json swap.
This ordering is intentional (preserve 3.x metadata while copying 4.x binaries) but non-obvious.- package_src = mklittlefs_dir / "package.json" - package_dst = mklittlefs4_dir / "package.json" + # Copy 3.x package.json into 4.x before mirroring 4.x -> 3.x, + # so 3.x dir ends up with 4.x binaries and 3.x metadata. + package_src = mklittlefs_dir / "package.json" + package_dst = mklittlefs4_dir / "package.json"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platform.py(13 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
platform.py
📚 Learning: 2025-07-29T11:35:26.182Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
Applied to files:
platform.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (macos-15, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (9)
platform.py (9)
37-38: Pathlib/typing imports: LGTM.
Clean, minimal additions to support the refactor.
99-101: IDF_TOOLS_PATH override matches project policy.
This aligns with your stated preference to always set IDF_TOOLS_PATH to PlatformIO core_dir. Good.
168-174: safe_copy_file: LGTM.
Creates parent dir and preserves metadata via copy2.
178-184: safe_copy_directory: LGTM.
dirs_exist_ok=Trueworks well for idempotent copies.
600-603: ULP detection via Path: LGTM.
Simple and readable.
619-627: Installer readiness check: LGTM.
Cleanup of legacy marker and existence check look correct.
899-899: Debug flash image check via Path: LGTM.
Simple existence guard; avoids load_cmds injection on missing images.
498-507: file:// URI construction is incorrect on Windows; use Path.as_uri().
f"file://{Path(...)}"yields two slashes and backslashes on Windows. This breaks local installs.- tl_path = f"file://{Path(IDF_TOOLS_PATH) / 'tools' / tool_name}" + tl_path = (Path(IDF_TOOLS_PATH) / "tools" / tool_name).as_uri()⛔ Skipped due to learnings
Learnt from: Jason2866 PR: pioarduino/platform-espressif32#263 File: platform.py:492-492 Timestamp: 2025-08-20T14:13:54.005Z Learning: In PlatformIO projects, `file://` is a special PlatformIO syntax for path references, not standard file URIs. The construction `f"file://{os.path.join(path, subpath)}"` is correct PlatformIO syntax and should not be replaced with pathlib.Path.as_uri().
198-203: Verify external usages of.packages_dir
No internal call sites rely on string-only operations (all wrap inPathor cast tostr). Ensure any downstream consumers expecting astrhandle the newPathreturn type.
Updated safe_remove_file to only check for files and symlinks before removal.
Refactor safe_remove_directory to handle symlinks and improve error handling.
Added comments to clarify the purpose of copying package.json.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor