-
Notifications
You must be signed in to change notification settings - Fork 279
Reworks balloon for sending balloons in freeze #3305
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: main
Are you sure you want to change the base?
Conversation
|
Mhh come to think of it, we don't really have the full old behavior anymore. Because now problems that are only solved in the freeze will never receive a balloon regardless of the value. What do you think, should the old config var be reinstated? |
60343b5 to
682de1f
Compare
|
Possibly related to #2778 |
Yes thank you! I was looking for this exact issue but couldn't find it for some reason. These indeed serve the same purpose. |
|
I'm quite sure the remarks phpstan makes are incorrect. Since otherwise it would not work at all while it does 🙃 |
Probably the docblocks for the methods being called are wrong. Use cd webapp
vendor/bin/phpstan analyze --configuration phpstan.dist.neonto check locally btw. |
Kevinjil
left a comment
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.
Just a general question: has something (e.g. at SEERC) influenced why we want this feature compared to the last time it was discussed IRL at the hackathon?
682de1f to
52d58f0
Compare
Partly. Some were just because we we're already looking at this code because of a bug (hence Nicky's PR) Some discussion and talks 'inspired' me on how to implement and that this is a nice feature. It was also the case that there were some teams that only solved some of the easy problems in the freeze. Because this is somewhat common -especially on the early rungs of the tournament ladder- it might be a nice motivational feature to add. The replacing of the old config flag came after (incorrectly) thinking that this change offered at least the same functionality as before. Tbh I cannot recall this discussion, but if we're opposed we can close the PR; I don't think it hurts anything while it does add a new functionality. When declining the pr we should cherry-pick 706147e since it DRYs up the retrieving of balloon updates. |
Balloons can be quite a motivator for a team and it can be frustrating to spend >4 hours solving a problem to only get an AC during the freeze. We used to have the show_balloons_postfreeze configflag to send balloons during the freeze but this would always send balloons during the freeze. show_balloons_postfreeze has now been replaced with 'minimum_number_of_balloons'. This setting now expresses the minimum number of balloons to send to a team even during the freeze. To prevent an information leak only balloons for problems that have been solved before the freeze can be sent out. Leaving this value to 0 keeps the 'old' behavior of not sending any balloons while setting it to a value >#contestproblems results in the old behaviour of sending all balloons during the freeze.
The computation for which balloons need to be sent out is a bit more complex than before. Reusing the logic of the BalloonService keeps it nice and DRY.
52d58f0 to
37f27e8
Compare
vmcj
left a comment
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.
I think we shouldn't merge this without tests. I do agree it happens relatively often but some in the team had strong opinions on such a feature, because of the same opinion we don't test this ourselves at competitions so I think if we ship this we should test it in a different way (with Unit tests for example..)
|
I'm not in favor of adding this because it adds extra complexity and weird exceptions. For example, what if a teams solves their first problem in the freeze, but it is an as yet unsolved problem? Highly unlikely maybe, but then that exposes crucial information during the freeze. There are likely similar edge-case issues, which I think is the reason we should not add this. |
|
I think a better way to solve this, is when |
So, this proposal basically shifts the balloon selection from a technical problem to an organizational problem. One could e.g. decide that such balloons are not handed out, unless the contest jury decides to grant it. |
I think the balloon runners are not always the most experienced with all the rules and edge cases on when something would disclose information. So in that case I would say we should also include that information (handing this out provides additional crucial information), and if you already have that information you also have the edge cases. So leaving the choice up to the balloon runners feels wrong to me. |
Replaces
show_balloons_postfreezewithminimum_number_of_balloonswhich allows for sending out balloons to teams even during the freeze while still supporting the old behavior if desired.This is useful as a motivational tool for teams that have not yet solved 'enough' problems before entering the freeze. e.g. 2 problems before the freeze while the expected number of problems to solve before the freeze is 3. By setting this new value to 3 the team will receive a balloon as long as the problem they've solved has been solved before the freeze.
Kept on draft until I fix the documentation and do some minor cleanup.