Skip to content

Conversation

@IllianiBird
Copy link
Collaborator

This PR fixes the following issue:

image

While I was doing so I also did the following:

  • Refactored MarginOfSuccess.java to remove unnecessary static methods. Why I made them in the first place is an answer lost to time. It probably seemed like a good idea at the time. It wasn't.
  • Added a test to ensure that we don't run into the issue of missing references for this class moving forward.

@IllianiBird IllianiBird self-assigned this Nov 19, 2025
@IllianiBird IllianiBird requested a review from a team as a code owner November 19, 2025 18:36
@IllianiBird IllianiBird added Bug GUI Tests Issues or Pull Requests related to the project tests Severity: Low Issues described as low severity as per the new issue form Text Issues related to text, translations, formatting labels Nov 19, 2025
@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 51.72414% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.72%. Comparing base (f130d04) to head (fd9ba83).
⚠️ Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
...mpaign/personnel/skills/AttributeCheckUtility.java 0.00% 4 Missing ⚠️
...paign/personnel/education/TrainingCombatTeams.java 0.00% 3 Missing ⚠️
...q/campaign/personnel/skills/SkillCheckUtility.java 25.00% 2 Missing and 1 partial ⚠️
...mpaign/personnel/skills/enums/MarginOfSuccess.java 87.50% 2 Missing ⚠️
...src/mekhq/campaign/personnel/skills/Appraisal.java 0.00% 1 Missing ⚠️
.../mekhq/campaign/personnel/skills/EscapeSkills.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8237      +/-   ##
============================================
- Coverage     11.72%   11.72%   -0.01%     
- Complexity     7532     7534       +2     
============================================
  Files          1271     1271              
  Lines        162724   162749      +25     
  Branches      24515    24520       +5     
============================================
+ Hits          19082    19083       +1     
- Misses       141644   141666      +22     
- Partials       1998     2000       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HammerGS HammerGS requested a review from Copilot November 19, 2025 19:08
Copilot finished reviewing on behalf of HammerGS November 19, 2025 19:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where margin of success labels were not being retrieved correctly from the resource bundle. The old implementation used numeric margin values (e.g., 4.label) as resource keys, but the resource bundle actually uses enum names (e.g., SPECTACULAR.label) as keys. The PR also refactors the MarginOfSuccess enum by converting unnecessary static methods to instance methods.

Key changes:

  • Fixed label retrieval by adding a lookupName field to store the correct resource bundle key
  • Refactored three static methods (getMarginValue, getMarginOfSuccessString, getMarginOfSuccessColor) to instance methods (getValue, getLabel, getColor)
  • Added a test to validate resource key references for all enum values

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
MekHQ/src/mekhq/campaign/personnel/skills/enums/MarginOfSuccess.java Fixed label retrieval by adding lookupName parameter and field; converted static methods to instance methods
MekHQ/unittests/mekhq/campaign/personnel/skills/enums/MarginOfSuccessTest.java Added new test to validate resource bundle keys for all enum values
MekHQ/src/mekhq/campaign/personnel/skills/SkillCheckUtility.java Updated to use instance methods instead of removed static methods; removed obsolete static imports
MekHQ/unittests/mekhq/campaign/personnel/skills/SkillCheckUtilityTest.java Updated test to use instance methods; removed obsolete static import
MekHQ/src/mekhq/campaign/personnel/skills/AttributeCheckUtility.java Updated to use instance methods instead of removed static methods; removed obsolete static imports
MekHQ/src/mekhq/campaign/personnel/skills/EscapeSkills.java Updated to use getColor() instance method instead of static getMarginOfSuccessColor()
MekHQ/src/mekhq/campaign/personnel/skills/Appraisal.java Updated to use getColor() instance method instead of static getMarginOfSuccessColor()
MekHQ/src/mekhq/campaign/personnel/education/TrainingCombatTeams.java Updated to use instance methods (getValue(), getLabel(), getColor()) instead of removed static methods; removed obsolete static imports

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug GUI Severity: Low Issues described as low severity as per the new issue form Tests Issues or Pull Requests related to the project tests Text Issues related to text, translations, formatting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant