Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup related to new naming scheme and refactoring, fixes refits to blank model names. #5031

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

Setsul
Copy link
Contributor

@Setsul Setsul commented Oct 11, 2024

Removed old kludges obsoleted by getFullChassis()
Refactoring of retrieveOriginalUnit() to enable
refactoring of populateRefits() that properly throws exceptions.

This fixes the bug that ever since (Standard) was purged from model names, those models with now blank names stopped showing up as targets for refits in the GUI.
And if something similar happens again we will get an EntityLoadingException now.

Screenshots from 49.7 (last milestone before the bug) vs 50.0 vs this commit for illustration purposes.
MekHQ refit bug3
MekHQ refit bug1
MekHQ refit bug5

Removed old kludges obsoleted by getFullChassis()
Refactoring of retrieveOriginalUnit() to enable
refactoring of populateRefits() that properly throws exceptions.
@IllianiCBT
Copy link
Collaborator

Welcome to the project :)

@IllianiCBT
Copy link
Collaborator

@WeaverThree given your recent work on refits, can you chuck your eyes over this PR and confirm if it looks good. Code-wise, I'm not seeing anything obviously wrong, but I've got zero experience with this corner of the code.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.46%. Comparing base (4e72790) to head (9173982).
Report is 81 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5031      +/-   ##
============================================
- Coverage     10.47%   10.46%   -0.01%     
+ Complexity     6029     6027       -2     
============================================
  Files           950      950              
  Lines        133397   133462      +65     
  Branches      19384    19390       +6     
============================================
- Hits          13974    13970       -4     
- Misses       118076   118146      +70     
+ Partials       1347     1346       -1     

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

@Setsul
Copy link
Contributor Author

Setsul commented Oct 11, 2024

Weaver did almost instantly point me at the line that was causing the problem, so credit where credit is due.

They did look over it already and chose this version over a literal 1 line fix that left the accumulated cruft in place, for obvious reasons, so I expect we'll get their official blessing shortly.

@WeaverThree
Copy link
Contributor

Looks good to me. I checked out retrieveOriginalUnit() and found that it's a single-client function used in Refit.saveCustomization(), which i checked and it still works fine :>. I also tested that it fixed the issue, and it did, it let me refit a Warhammer IIC 2 to a plain Warhammer IIC no problem.

@IllianiCBT
Copy link
Collaborator

Perfect, thanks for taking a look :)

@IllianiCBT IllianiCBT merged commit a3bf93e into MegaMek:master Oct 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants