Skip to content

[core] read forced_share_percent from options#12745

Open
kepakiano wants to merge 2 commits into
tobymao:masterfrom
kepakiano:core_corporation_read_forced_share_percent_from_options
Open

[core] read forced_share_percent from options#12745
kepakiano wants to merge 2 commits into
tobymao:masterfrom
kepakiano:core_corporation_read_forced_share_percent_from_options

Conversation

@kepakiano

Copy link
Copy Markdown
Contributor

refers to no issue, but it was mentioned in #12727 that not reading forced_share_percent from the options was probably an oversight and multiple games seem to be assuming that it gets properly set. There is code setting this in the entities.rbfor 1822 Africa, 1854, and 1847.

Before clicking "Create"

  • Branch is derived from the latest master
  • Add the pins or archive_alpha_games label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

@perwestling perwestling left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

PR Review: #12745 - [core] read forced_share_percent from options

Reviewed with AI assistance by Mistral Vibe


✅ Strengths

  • Fixes a real issue: forced_share_percent option wasn't being read from entity definitions
  • Consolidates code: Removes hardcoded settings in g_1835/game.rb
  • Aligns with existing patterns: 1822 Africa, 1854, 1847 already use this option
  • All tests pass

🚨 Blocking Issue

  • Line 81 of corporation.rb: Typo in option key (:@forced_share_percent:forced_share_percent)

⚠️ Concerns

  • Missing label: PR description says to add pins or archive_alpha_games label if this breaks existing games. Since this is a core change to Corporation initialization, please add the appropriate label.

💡 Suggestions

  • Add tests verifying forced_share_percent is read correctly from options
  • Consider if other options might have similar issues

❓ Questions

  • Have you tested that corporations in 1835, 1822 Africa, 1854, and 1847 still work correctly with this change?

❌ Verdict: Request changes

The typo on line 81 of corporation.rb must be fixed before merge. Once corrected and the label is added, this will be ready to approve.

Comment thread lib/engine/corporation.rb Outdated
@reservation_color = opts[:reservation_color]
@price_percent = opts[:price_percent] || @second_share&.percent || (@presidents_share.percent / 2)
@price_multiplier = (@second_share&.percent || (@presidents_share.percent / 2)) / @price_percent
@forced_share_percent = opts[:@forced_share_percent]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking issue: Typo in option key

The key should be :forced_share_percent not :@forced_share_percent:

@forced_share_percent = opts[:forced_share_percent]

The @ prefix in the symbol key is incorrect. Compare with other options in the same method:

  • opts[:price_percent]
  • opts[:reservation_color]
  • opts[:treasury_as_holding]

All use symbols without the @ prefix. This typo will cause the option to never be read from entities.

@kepakiano

Copy link
Copy Markdown
Contributor Author

@perwestling what do you think? I don't know any of the other affected games well. Could reading this flag lead to a change in their default share percentage (e.g. from 20 to 10) and thus in wrong amounts of money being transferred?
Ah, damn, I'd relied on the other games having tests. But I see now that 1854 has no tests, the 1822Africa tests don't cover money and the 1847 fixtures don't have explicit tests.

Nevermind any of that, I see the pipeline failing with 1822Africa tests being red. Do you know if Alexander Vansach is still active? He seems to be the original developer of 1822Africa and I'd like to talk to him or someone else about these fixtures.

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.

2 participants