Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1667 +/- ##
==========================================
- Coverage 54.67% 54.16% -0.51%
==========================================
Files 336 340 +4
Lines 27450 27787 +337
==========================================
+ Hits 15009 15052 +43
- Misses 12441 12735 +294
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, there should be some documentation of the controller communication format. Also, i'm not sure i like the name: i think it'd be more descriptive to call this something like "DynamicInteractive" to emphasize that it spawns solutions dynamically. (and then Communication could be renamed to Interactive, which is what it really is. though we'd probably need to keep the name Communication as an alias.) |
The plan is to have Interactive eventually also take over the tasks that we currently do with Communication, so I am happy with the "Interactive" task name. As for documentation: that will come, I would like first to make sure that we agree the implementation is sensible. |
prandla
left a comment
prandla
left a comment
There was a problem hiding this comment.
i think the implementation in general is fine now. interactive_keeper.py doesn't really spark joy for me, but i don't know how to improve it much. (aside from the one comment i left in it)
| return score, text, admin_text | ||
|
|
||
|
|
||
| class CopyAbsPathFileCacher: |
There was a problem hiding this comment.
this feels really ugly....
i think you could just not give the Sandbox any FileCacher. then you'd have to use create_file_from_string or plain create_file instead of create_file_from_storage, but it would still feel a lot less hacky to me.
| """Type for an integer parameter.""" | ||
|
|
||
| TEMPLATE = GLOBAL_ENVIRONMENT.from_string(""" | ||
| <select name="{{ prefix ~ parameter.short_name }}"> |
There was a problem hiding this comment.
a dropdown with the options "true" and "false"? have you ever heard of a checkbox? :)
also, the docstring just above seems to be a copy-paste error, should say "boolean parameter". (unless you're subtly alluding to the fact that bool is a subclass of int :p)
There was a problem hiding this comment.
I fixed the docstring.
However, it's at least non-trivial to use a checkbox, since for whatever reason a checkbox does not send a value if the checkbox is unchecked, and the existing infra expects a value.
There was a problem hiding this comment.
yeah, that's just how html checkboxes work, it is a bit annoying.
i'd still prefer if this was fixed. a checkbox is the semantically correct input for a boolean. you don't need to change existing infra much: just override parse_handler instead of parse_string. (i think you still need to provide a dummy parse_string to make it a non-abstract class though)
2 participants
No description provided.