fix: detect core features properly#1147
Conversation
WalkthroughThe changes refine minimal version string handling by conditionally removing trailing “.0” only when present. VR feature list construction is made conditional on VR module activation and developer mode, with caching to reflect state changes. The test INI marker file mechanism is fully removed, simplifying test INI management and cleanup logic. Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates feature issue handling to treat VR as a core feature and removes the use of test marker paths. Key changes include:
- Removal of the testMarkerPath field and associated logic in FeatureIssues.h and FeatureIssues.cpp.
- Updates to logging and issue message formatting.
- Adjustment of feature filtering behavior for VR support in Feature.cpp.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/FeatureIssues.h | Removed testMarkerPath member from TestIniInfo to align with updated design. |
| src/FeatureIssues.cpp | Removed marker file creation/deletion and adjusted logging/messaging accordingly. |
| src/Feature.cpp | Added default minimal version for core features and refined VR filtering logic. |
Comments suppressed due to low confidence (3)
src/FeatureIssues.h:189
- The removal of the testMarkerPath member should be fully propagated; please ensure that all related persistence and logging behavior referencing this field are updated accordingly to avoid inconsistencies.
std::string testMarkerPath; // Path to .test marker file for tracking (new files only)
src/FeatureIssues.cpp:819
- With the removal of testMarkerPath, the wasManuallyDeleted() logic has changed; please confirm that the new logic correctly handles cases where markers previously influenced detection of manual deletion.
return !std::filesystem::exists(testIniPath);
src/Feature.cpp:225
- [nitpick] The VR-specific feature filtering logic now always returns featuresVR; please verify that in developer mode the intended behavior (keeping all features) is maintained and clearly documented.
if (REL::Module::IsVR()) {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/FeatureIssues.cpp (1)
817-820:wasManuallyDeleted()now duplicates!stillExists()– consider removing one of themAfter dropping the marker-file logic,
stillExists()andwasManuallyDeleted()have become strict negations of each other.
That adds zero information and complicates maintenance/readability. Prefer a single predicate, e.g.:-bool TestIniInfo::stillExists() const -{ - return std::filesystem::exists(testIniPath); -} - -bool TestIniInfo::wasManuallyDeleted() const -{ - return !std::filesystem::exists(testIniPath); -} +bool TestIniInfo::IsPresent() const +{ + return std::filesystem::exists(testIniPath); +}…or simply inline the check at call-sites.
This keeps the API minimal and avoids “two ways of saying the same thing”.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Feature.cpp(2 hunks)src/FeatureIssues.cpp(2 hunks)src/FeatureIssues.h(0 hunks)
💤 Files with no reviewable changes (1)
- src/FeatureIssues.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Validate PR title
- GitHub Check: Check for changes in PRs
🔇 Additional comments (1)
src/Feature.cpp (1)
72-88: Risk of silently accepting out-of-date core featuresWhen a core feature is missing from
FEATURE_MINIMAL_VERSIONS, the code now falls back to a hard-coded1.0.0:minimalVersion = REL::Version("1.0.0");If the real minimal version later becomes
1.2.0, older installs will pass the check unnoticed, defeating the intent of the map.Recommend either
- adding every core feature to the map (preferred) and treating absence as a programming error (e.g.
SKSE::log::critical+ abort), or- deriving the fallback from the DLL’s own version instead of a constant.
Fail-fast here avoids hard-to-trace mismatches in the field.
alandtse
force-pushed
the
core_feature_issues
branch
from
bc8cbc3 to
bf33ccb
Compare
alandtse
changed the title
|
✅ A pre-release build is available for this PR: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Feature.cpp (1)
91-93:⚠️ Potential issueFix unconditional trim – non-zero patch numbers are still cut off
This block still strips the last two characters of
minimalVersionStringregardless of the patch value, undoing the safety check you added later (lines 131-136).
Users will again see misleading requirements like1.2instead of1.2.4.-std::string minimalVersionString = minimalFeatureVersion.string(); -minimalVersionString = minimalVersionString.substr(0, minimalVersionString.size() - 2); +std::string minimalVersionString = minimalFeatureVersion.string(); +if (minimalVersionString.ends_with(".0")) { // remove only a trailing “.0” + minimalVersionString.erase(minimalVersionString.size() - 2); +}
🧹 Nitpick comments (1)
src/FeatureIssues.cpp (1)
817-820:wasManuallyDeleted()now mirrors!stillExists()— consider consolidationSince both methods are logical complements (
stillExists()→exists,wasManuallyDeleted()→!exists), the dual calls in the stats loop (lines 910-919) are now redundant.
Either drop one of the methods or rewritewasManuallyDeleted()to convey extra intent (e.g., track previous existence via persisted state).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Feature.cpp(2 hunks)src/FeatureIssues.cpp(1 hunks)src/FeatureIssues.h(0 hunks)
💤 Files with no reviewable changes (1)
- src/FeatureIssues.h
🔇 Additional comments (1)
src/Feature.cpp (1)
218-245: Dynamic VR-aware cache looks solid👍 The new
BuildVRListhelper together withfeaturesVRandcachedDevModecorrectly rebuilds the list when Developer Mode changes, eliminating the stale-cache issue flagged earlier. No further action required.
fix: detect core features properly (doodlum#1147)
Summary by CodeRabbit
Bug Fixes
Refactor
Chores