-
Notifications
You must be signed in to change notification settings - Fork 194
[1880] Add log message after SR when corp can't buy train #11691
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
base: master
Are you sure you want to change the base?
[1880] Add log message after SR when corp can't buy train #11691
Conversation
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.
Does this work if a company had bought the last train of a type, triggering the stock round? I suspect that log_skip
might not be called in that case.
You were correct. I added a log_pass method that checks if the id of the first available train from the depot ends in '-0', as well as the previous checks, to return the same messages. By the way, the initial change has the side effect of also returning the log message when the buy_train step is skipped normally for the reasons of not having room or enough money to buy a train. Personally, I think it's an improvement, and something that we could even consider adding to the base code. But for now I've just left it to this game, |
This is going to produce inconsistent logging. For example if a company buys a train from a friend and hits train limit then you may or may not see the log message about being at train limit, depending on which is the next train in the depot. And, IMHO, we don't generally need to be adding more to the log, it's noisy enough as is. I would have thought that a better place to try and add the log message would be before the 18xx/lib/engine/game/g_1880/round/operating.rb Lines 10 to 19 in ea449a2
|
Wow. Not sure how I missed that! I'll agree that's definitely the right place for it, and significantly easier too. |
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.
Setting this PR to changes requested, so it doesn't get merged until it has been updated.
Fixes #8808
Before clicking "Create"
master
pins
orarchive_alpha_games
label if this change will break existing gamesdocker compose exec rack rubocop -a
docker compose exec rack rake
Implementation Notes
Explanation of Change
Initially I was going to add this log message only if the corp was coming out of an SR, but I wasn't sure that I was really limiting it to that scenario. I realised though, these added log messages works fine even if the corp isn't coming out of an SR, so I simplified the code.
I don't even think
super
is needed at this point, but I figured I'd leave it as a catch-allScreenshots
Any Assumptions / Hacks