Round calculated colours to protect against floating point precision errors; options defaults consistency#103
Conversation
…errors. Moved IsDeathKnight into ClassUtil. Removed unused ColorUtil.AreEqual
Test Results910 tests +17 910 ✅ +17 2s ⏱️ -1s Results for commit 3ef343d. ± Comparison against base commit d91a04b. This pull request removes 236 and adds 253 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR refactors class detection and constant definitions while tightening color/float-related comparisons by removing an unused color equality helper and updating affected tests/call sites.
Changes:
- Moved
IsDeathKnightfromECM.luaintoClassUtil.luaand updated all consumers (UI + module + tests). - Removed unused
ColorUtil.AreEqualand adjusted tests that previously validated or stubbed it. - Refactored
Constants.luato directly expose several constant tables (and split module load order from chain order), updating runtime enable logic to useMODULE_LOAD_ORDER.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| UI/RuneBarOptions.lua | Switches DK gating to ns.ClassUtil.IsDeathKnight() for RuneBar options visibility/disable logic. |
| UI/ResourceBarOptions.lua | Updates ResourceBar options disable predicate to ns.ClassUtil.IsDeathKnight. |
| Tests/TestHelpers.lua | Updates options test environment to provide ns.ClassUtil.IsDeathKnight (instead of ns.IsDeathKnight). |
| Tests/Modules/RuneBar_spec.lua | Updates RuneBar tests to stub ClassUtil.IsDeathKnight. |
| Tests/FrameUtil_spec.lua | Removes now-unneeded ns.ColorUtil.AreEqual stub from FrameUtil tests. |
| Tests/ColorUtil_spec.lua | Removes AreEqual test and updates test harness setup (nit noted in review comment). |
| Tests/ClassUtil_spec.lua | Adds unit tests for ClassUtil.IsDeathKnight. |
| Runtime.lua | Switches module enable iteration to C.MODULE_LOAD_ORDER. |
| Modules/RuneBar.lua | Updates DK checks to ns.ClassUtil.IsDeathKnight(). |
| EnhancedCooldownManager.code-workspace | Adjusts Lua diagnostics globals list. |
| ECM.lua | Removes ns.IsDeathKnight implementation from root namespace. |
| Constants.lua | Exposes constant tables directly on constants and introduces MODULE_LOAD_ORDER. |
| ColorUtil.lua | Removes unused ColorUtil.AreEqual. |
| ClassUtil.lua | Adds ClassUtil.IsDeathKnight and keeps class/resource utilities centralized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…tBox). Moves export and import profile buttons to the pageActions section.
Owner
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c095da4899
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+384
to
+386
| local function getDefaultsConfirmation(builder) | ||
| local confirmation = builder._config.defaultsConfirmation | ||
| assert(type(confirmation) == "table", "LibSettingsBuilder: defaultsConfirmation is required when page defaults are enabled") |
There was a problem hiding this comment.
Existing LibSettingsBuilder consumers can pass the previously supported defaultsConfirmation(pageName, onAccept) callback, but any page with onDefault or collected default settings now reaches this assertion and aborts registration because only a table is accepted. Since the library remains LibSettingsBuilder-1.0, support the callback form or introduce this as a versioned breaking API change.
Useful? React with 👍 / 👎.
argium
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves LUA error when creating a new profile. #105
Resolves inconsistencies with profile defaults.
Moved IsDeathKnight into ClassUtil.
Removed unused ColorUtil.AreEqual