-
Notifications
You must be signed in to change notification settings - Fork 295
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
Implement RFE 6147: move infantry loading to interface and allow cargo bays #6627
base: master
Are you sure you want to change the base?
Implement RFE 6147: move infantry loading to interface and allow cargo bays #6627
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6627 +/- ##
==========================================
Coverage 29.15% 29.16%
- Complexity 15415 15444 +29
==========================================
Files 2865 2871 +6
Lines 280027 280232 +205
Branches 49307 49335 +28
==========================================
+ Hits 81655 81717 +62
- Misses 192890 193030 +140
- Partials 5482 5485 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm really excited for this. Code-wise everything looks great. Missing license file on CargoBayTest.java
will need added, and I think the javadoc for InfantryTransporter.java
was copied from Transporter.java
- I was confused by it for a moment while reviewing, so maybe that should be looked at.
Thanks!
/** | ||
* Classes that implement this interface have the ability to load, carry, and | ||
* unload units in the game. It is anticipated that classes will exist for | ||
* passenger compartments, battle armor steps, Mek bays, Aerospace hangers, and | ||
* vehicle garages. Other possible classes include cargo bays and Dropship | ||
* docks. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be updated? I think even including a reference to Transporter (@see Transporter
) would help someone unfamiliar with MM's transport system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License?
There are a few more
I don't think this should be implemented for the BA Handles even though those are for Transporting infantry, since this is specifically for infantry that are using their weight for loading. |
See https://discord.com/channels/458705327911731231/1344135419180351560 for rationale for some design decisions taken here.
Testing:
Merging this should unblock MegaMek/mekhq#5929
Close #6147