fix: Garrison fixes and minor changes#475
Conversation
Reviewer's Guide by SourceryThis pull request implements several fixes for the garrison system by updating the disposition description logic with added debug checks, adjusting the garrison report logic based on planet owner, refining charisma test handling to adjust disposition changes, and adding safeguards to avoid unexpected disposition values in PlanetData. The changes aim to prevent negative disposition values and ensure consistent behavior during edge cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @MCPO-Spartan-117 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider centralizing the repeated disposition clamping logic (e.g. for values below 0 and above 100) into a dedicated helper function to reduce duplication.
- Clarify the threshold conditions in the charisma test adjustments, as the combined comparisons and use of abs() could benefit from explicit explanation for maintainability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
OH296
left a comment
There was a problem hiding this comment.
I'm good with this few questions that may need resolving though. Most likely i think reverting that hard minimum of 0 to -100 should be sufficient though
Blogaugis
left a comment
There was a problem hiding this comment.
I don't see any issues at the first glance, so here's my approval.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @MCPO-Spartan-117 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Clarify the conditional logic in the charisma test block of GarrisonForce, especially around the adjustment of dispo_change when nearing the -100 and 100 boundaries.
- Explain or refactor the disposition clamping values in PlanetData to make the intended thresholds and special cases more transparent.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @MCPO-Spartan-117 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider unifying numeric boundaries for disposition values across the code to ensure consistency between debug messages and value adjustments.
- Remove or conditionally compile debug logs and commented-out branches so that production code is cleaner and more maintainable.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIn these changes, a new conditional logic block has been integrated into the PlanetData constructor. The code now checks if a planet’s disposition value is negative but greater than -1000 and confirms that the planet’s owner is not the player faction; if so, the disposition is set to -100. Additionally, any disposition exceeding 100 is capped at 100. In the garrison module, modifications have been made to the Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
Description of changes
Reasons for changes
How have you tested your changes?