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

[18Uruguay] Also add FCE shares to secondary corps #11209

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

patrikolesen
Copy link
Contributor

@patrikolesen patrikolesen commented Sep 12, 2024

Fixes #11111
Fixes #11074

Fixes #[PUT_ISSUE_NUMBER_HERE]

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
    This can break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

Explanation of Change

Screenshots

Any Assumptions / Hacks

@ollybh ollybh added archive_alpha_games Needs alpha games archiving 18Uruguay labels Sep 12, 2024
Copy link
Collaborator

@ollybh ollybh left a comment

Choose a reason for hiding this comment

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

This is looking much better. I've suggested a couple more tweaks to the code, nothing major this time.

lib/engine/game/g_18_uruguay/nationalization.rb Outdated Show resolved Hide resolved
lib/engine/game/g_18_uruguay/nationalization.rb Outdated Show resolved Hide resolved
@ollybh
Copy link
Collaborator

ollybh commented Dec 15, 2024

@patrikolesen This pull request has been open for a while now. Has there been any progress? Just checking that you haven't forgotten about this one…

@patrikolesen
Copy link
Contributor Author

Thanks for the ping, I will look at this the comming days

@@ -28,10 +28,13 @@ def acquire_shares
entity_shares = affected_shares(holder, @merge_data[:corps])

total_percent = entity_shares.sum(&:percent)
num_shares, odd_shares = (total_percent / 10).divmod(2)
from_secondary = @merge_data[:secondary_corps].count { |corp| corp.president?(holder) }
num_shares = (total_percent / 20).to_i
Copy link
Collaborator

Choose a reason for hiding this comment

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

@patrikolesen Did you mean to revert this change in this commit? What was rubocop objecting to here? Or was this commit just supposed to remove the space after bundle = below?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
18Uruguay archive_alpha_games Needs alpha games archiving
Projects
None yet
2 participants