Allow configuring subtasks to always show testcases.#1677
Allow configuring subtasks to always show testcases.#1677veluca93 wants to merge 1 commit intocms-dev:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1677 +/- ##
==========================================
+ Coverage 54.68% 54.71% +0.02%
==========================================
Files 336 336
Lines 27439 27464 +25
==========================================
+ Hits 15006 15026 +20
- Misses 12433 12438 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Also defines a new (optional) format for parameters for ScoreTypeGroup. Also allows specifying testcases by lists instead of by regex.
veluca93
force-pushed
the
always_show_testcases
branch
from
b183593 to
7274ee0
Compare
This includes allowing to configure per-subtask "always show testcases" introduced in cms-dev/cms#1677.
prandla
left a comment
prandla
left a comment
There was a problem hiding this comment.
Some philosophical comments regarding type hints.
The implementation itself looks fine.
| class ScoreTypeGroupParametersDict(TypedDict): | ||
| max_score: float | ||
| testcases: int | str | list[str] | ||
| threshold: NotRequired[float] |
There was a problem hiding this comment.
i'm a little unhappy about this scoretype-specific field being here, but well, the typing of the parameters is a huge mess anyways and this is hardly the worst part of it.
i don't think it's possible to fix it without refactoring how scoretypes work in general. python doesn't have associated types, so i don't think there is even a good way to express "this scoretype uses this subtype of the parameters dict for its methods". so we can just continue ignoring the type errors.
| {% endfor %}""" | ||
|
|
||
| def get_max_score(self, group_parameter: ScoreTypeGroupParameters) -> float: | ||
| if isinstance(group_parameter, tuple) or isinstance(group_parameter, list): |
There was a problem hiding this comment.
can these ever legitemately be tuples? these parameters should be loaded from json, which doesn't return tuples. the type hint says tuple only because there is no way to type-hint a list whose elements have different types 🙃 (actually, that should probably be added to the comment near the definition of ScoreTypeGroupParameters)
if you assumed doing an isinstance(tuple) here would get the type checker to behave on the next line, then unfortunately that doesn't appear to be the case, as pyright at least assumes that the parameter is some kind of magical intersection of list and ScoreTypeGroupParametersDict.
|
I also briefly considered whether we should drop support for the list-based format entirely (at least within cms.grading.scoretypes) and convert the lists to dicts in the importers / a db updater, but it'd probably end up being more effort in total. that would probably also be more disruptive to forks with custom scoretypes. |
2 participants
Also defines a new (optional) format for parameters for ScoreTypeGroup.