Conversation
…tive_java_home for cache keys; replace repeated symlink resolution with helper in java_find; remove redundant version-sorting
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 85.97% 86.18% +0.20%
==========================================
Files 20 20
Lines 1954 1947 -7
==========================================
- Hits 1680 1678 -2
+ Misses 274 269 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors symlink resolution logic, fixes pkgdown configuration for newer versions, and corrects caching behavior in Java version check functions.
Key changes:
- Extracts duplicate symlink resolution code into a new
resolve_symlinks()utility function with comprehensive test coverage - Updates
_pkgdown.ymlto use current navbar syntax (moving from deprecatednavbar:fields in articles to explicitnavbar: components:structure) - Fixes cache key logic in
java_check_version_rjava()andjava_check_version_cmd()to use the actualjava_homebeing checked rather than always usingSys.getenv("JAVA_HOME")
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-internal_utilities_symlinks.R | Adds comprehensive test suite for the new resolve_symlinks() function covering edge cases like circular symlinks, relative paths, and broken links |
| _pkgdown.yml | Migrates from deprecated pkgdown syntax to current best practices by defining article menu structure in navbar: components: instead of inline navbar: fields |
| R/java_find.R | Refactors duplicate symlink resolution code to use new resolve_symlinks() utility and removes redundant sorting from cached function |
| R/java_env.R | Improves path normalization using normalizePath() instead of manual string replacement, and fixes cache keys to use the effective java_home being checked |
| R/internal_utilities.R | Adds new resolve_symlinks() utility function with proper NA handling and support for relative symlinks, circular references, and configurable max depth |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Get check result (either cached or fresh) | ||
| cache_key <- Sys.getenv("JAVA_HOME") | ||
| # Use the effective java_home as cache key (what we're actually checking) |
There was a problem hiding this comment.
The intermediate variable effective_java_home is assigned directly from java_home without any transformation (unlike in java_check_version_cmd where it has conditional logic). While this provides naming consistency between the two functions, you could consider directly using java_home as the cache key parameter to reduce indirection, or add a comment explaining why the intermediate variable exists for consistency with java_check_version_cmd.
| # Use the effective java_home as cache key (what we're actually checking) | |
| # Use the effective java_home as cache key (what we're actually checking). | |
| # Note: In this function, effective_java_home is identical to java_home, | |
| # but we keep a separate variable for consistency with java_check_version_cmd, | |
| # where effective_java_home may differ and is used as the cache key. |
No description provided.