[catsoop-dev] Allow returning attempts after syntax errors in expressions

[adam j hartz]

Hi Arjun,

Thanks for sending this!  I think it's definitely on the right track.
I'm including some thoughts below, and I'm certainly happy to continue
the conversation.

Thanks,
-Adam



> diff --git a/catsoop/__HANDLERS__/default/default.py b/catsoop/__HANDLERS__/default/default.py
> index e82f7b69..00eb7e94 100644
> --- a/catsoop/__HANDLERS__/default/default.py
> +++ b/catsoop/__HANDLERS__/default/default.py
> @@ -1294,11 +1294,13 @@ def handle_submit(context):
>                          context, resp["msg"]
>                      )
>                      extra = resp.get("extra_data", None)
> +                    free_attempt = resp.get("return_attempt", False)
>                  except:
>                      resp = {}
>                      score = 0.0
>                      msg = exc_message(context)
>                      extra = None
> +                    free_attempt = False
>                  out["score"] = scores[name] = newstate.setdefault("scores", {})[
>                      name

This looks good to me.  It will require some thought, though, to
integrate this into the asynchronous checker as well, since we won't
know whether we want to refund the submission until after the checker
completes.

So this probably needs a change in scripts/checker.py (that handles the
asynchronous checking) as well, so that we can make that refund happen
when using the async checker.  Probably as we're building up the `row`
structure in that script (around line 141), we want to check whether we
should refund the submission and, if so, go ahead and update the log
from inside of that script.


I also think it would be great for cs_check_function to be able to
include this new key ("return_attempt") in their return values, and for
us to respect that as an override to whatever the default behavior is.
While it's nice to be able to specify whether things should be refunded
on an _error_ (and I think we should keep that), it would be great if in
my individual check function I could also say that I don't want a
submission to be counted, for some reason or another.

Unfortunately, given how things are currently structured, that probably
involves either making a bunch of additional changes to the built-in
question types in the __QTYPES__ folder (where they build up the
dictionaries with "score" and "msg", we should also include
"return_attempt"), or rethinking how we organize those things altogether
(it's not great that the code for handling the different possible return
types of the check functions exists in every question type, so that
should probably be reorganized at some point, though not necessarily as
part of this change).


> diff --git a/catsoop/__QTYPES__/expression/expression.py b/catsoop/__QTYPES__/expression/expression.py
> index 7f4cacc2..a5976a58 100644
> --- a/catsoop/__QTYPES__/expression/expression.py
> +++ b/catsoop/__QTYPES__/expression/expression.py
> @@ -75,6 +75,7 @@ allow_save = False
>
>  defaults = {
>      "csq_error_on_unknown_variable": False,
> +    "csq_use_attempt_on_error": True,
>      "csq_input_check": lambda raw, tree: None,
>      "csq_render_result": True,
>      "csq_syntax": "base",
> @@ -495,7 +499,7 @@ def handle_submission(submissions, **info):
>              msg += get_display(info, n, sub, False, _m or "")
>          else:
>              msg += _m or ""
> -        return {"score": result, "msg": msg}
> +        return {"score": result, "msg": msg, "return_attempt": return_attempt}

One thing I'm a little unsure of is the names "use_attempt_on_error" and
"return_attempt".  I feel like those should probably have the same
polarity (so that we don't need the `not` to convert between them.
Beyond that, "return" is also somewhat overloaded, and we also don't use
the word "attempt" anywhere else in the codebase, I don't think.  So
maybe we want to use "count_submission" and "count_submission_on_error"
or something like that?

Alternatively, if we wanted to use the opposite structure, I think I
like "refund" instead of "return."  But I would still think the two
variables should have the same name, but then it would be like
"refund_submission" and "refund_submission_on_error".  I think I like
that better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://mailman.mit.edu/pipermail/catsoop-dev/attachments/20240421/3891ef45/attachment-0001.sig>