-
Notifications
You must be signed in to change notification settings - Fork 12
Fix Secret vs hidden teams #241
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?
Conversation
deboer-tim
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.
Looking backward I am not happy that we went from hidden groups to hidden teams to a default scoreboard group 😞, and it seems slightly more annoying for CCSs and CDS to implement. e.g. 'is this team on the scoreboard' is indirect now. That said, I think this is simpler and more consistent with the rest of the spec. 👍🏼
| | scoreboard\_freeze\_duration | RELTIME ? | How long the scoreboard is frozen before the end of the contest. Defaults to `0:00:00`. | ||
| | scoreboard\_thaw\_time | TIME ? | The scheduled thaw time of the contest, may be `null` if the thaw time is unknown or not set. | ||
| | scoreboard\_type | string | What type of scoreboard is used for the contest. Must be either `pass-fail` or `score`. | ||
| | default_scoreboard_group | ID ? | Identifier of the group that represents the default scoreboard. If `null`, the default scoreboard contains all teams (even if there is no group containing all teams). |
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 don't love the name (kinda long), but should also end in _id to match other fields like this. And I guess we should use backslashes in markup too?
Maybe just scoreboard\_group\_id to match the others? In one sense it is the default, but it's also overriding the default (all teams) behaviour too, so I'd lean more toward matching other property names.
I think the term 'default scoreboard' is potentially confusing too. I've never thought of the current scoreboard as the 'default' one - it's just 'the' scoreboard, or the 'primary' scoreboard when compared to group-specific ones. So I'd lean toward replacing:
Identifier of the group that represents the default scoreboard. If null, the default scoreboard contains all teams (even if there...
with something simpler like:
Identifier of the group of teams that are on the scoreboard. If null, the scoreboard contains all teams.
|
|
||
| `contests/<id>/scoreboard?group_id=site1` | ||
|
|
||
| If no `group_id` is given, the group specified by `deafault_scoreboard_group` in the contest endpoint is used, or all teams if it is `null`. |
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.
Typo in property name (and update if we change the name above).
I might be overly picky, but the current setup is that group_id is an optional parameter - you only add it when you want a specific group. To me this change makes it sound more like the user is expected to pass the group_id and we're specifying a default behaviour for when they don't. I'm ok if we want to include a mention of the default group id in the Scoreboard section, but I'd do that in the main part. The section on group_id shouldn't need to change b/c picking the same group as the default is maybe unnecessary, but isn't special in any way.
Continuation of #240.
Second half of #230
Closes #230.