Skip to content

Give favoured and repressed guilds their own base effects - #69

Merged
JustinasLa merged 1 commit into
mainfrom
fix/favour-repress-base-effects
Sep 28, 2026
Merged

JustinasLa merged 1 commit into
mainfrom
fix/favour-repress-base-effects

Conversation

@JustinasLa

@JustinasLa JustinasLa commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

The scope switch in LawHandler.getLawModifiers had no breaks, so a domestic guild fell through to the vassal cases:

configured (base-effects) applied before this fix
Favoured guild favoured_guilds: +20% trade in our territory favoured_vassals: +15%, plus the vassal tax_multiplier(-5)
Repressed guild repressed_guilds: −15% trade in our territory repressed_vassals: −10%, plus the vassal tax_multiplier(5)

The live server shows the old behaviour: YevaKeepers (favoured in TheHolyOrder) only matches the exported province_data.json when the +15% vassal effect is applied.

  • Break after the DOMESTIC_GUILDS case
  • VASSALS and VASSAL_GUILDS share one case
  • Add FavourRepressBaseEffectTest covering all four base effects. Two of its tests fail on main (15 vs 20, −10 vs −15)

Full suite: 2303 tests pass locally with mvn clean verify.

Deploying this changes income for every nation that favours or represses guilds. A favoured guild gets more trade share in its home territory, and a repressed guild loses more.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected how law modifiers are applied to domestic guilds, vassals, and vassal guilds. Domestic guilds no longer inherit vassal-related modifiers, while vassals and their guilds receive the appropriate favoured or repressed scope.

The scope switch in LawHandler.getLawModifiers had no breaks, so a
domestic guild fell through to the vassal cases. Favoured guilds got
favoured_vassals (+15% trade) instead of favoured_guilds (+20%), and
repressed guilds got repressed_vassals (-10%) instead of
repressed_guilds (-15%).

- Break after the DOMESTIC_GUILDS case
- VASSALS and VASSAL_GUILDS share one case
- Add FavourRepressBaseEffectTest for all four base effects

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b48b98c1-245f-4ffe-86bb-953a27c590ca

📥 Commits

Reviewing files that changed from the base of the PR and between 5928237 and 779f4ee.

📒 Files selected for processing (2)
  • src/main/java/net/tfminecraft/simplefactions/objects/handler/LawHandler.java
  • src/test/java/net/tfminecraft/simplefactions/laws/FavourRepressBaseEffectTest.java

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

LawHandler now selects the correct modifier scope for domestic guilds, vassals, and vassal guilds. Tests check trade-power base effects for favoured, repressed, and ordinary guilds.

Changes

Law modifier scope

Layer / File(s) Summary
Modifier scope selection and tests
src/main/java/net/tfminecraft/simplefactions/objects/handler/LawHandler.java, src/test/java/net/tfminecraft/simplefactions/laws/FavourRepressBaseEffectTest.java
DOMESTIC_GUILDS now exits its switch branch. VASSALS and VASSAL_GUILDS share vassal-scope selection. Tests check the base effects for favoured and repressed guilds and vassals, and verify that an ordinary domestic guild receives no base effect.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: ryanbarlow97

Merge Risk: ⚪ Minimal · up to 779f4

No actionable merge-blocking risk is identified; the intended guild income change is ready for normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: assigning configured base effects to favoured and repressed guilds.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

A rabbit checks the guilds at play,
Favoured scopes now find their way.
Vassals share the proper line,
Domestic cases end in time.
The trade-power tests all hop along,
With carrots for a closing song.

Comment @coderabbitai help to get the list of available commands.

@JustinasLa
JustinasLa merged commit bb2a8a0 into main Sep 28, 2026
2 checks passed
@JustinasLa
JustinasLa deleted the fix/favour-repress-base-effects branch September 28, 2026 00:33
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.

1 participant