Skip to content
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

Bug: grader report may display negative numbers without mandatory parenthesis around them #440

Closed
letouzey opened this issue Oct 7, 2021 · 1 comment · Fixed by #448
Closed
Assignees

Comments

@letouzey
Copy link

letouzey commented Oct 7, 2021

Bug description

Let's consider a exercise about a function foo with at least one int argument.
If the tests about this function (e.g. Test_lib.test_function_1) may involve negative integers,
then the grader report will display these negative numbers without parenthesis. This leads to
incorrect code fragment like foo -6 in the report :

Computing foo -6  : 1 pt
Correct value 6

This isn't just a cosmetic error, it actually prevent a student from copy-pasting these
fragments unedited in an OCaml toplevel for debugging purpose (the actual situation
was involving a function with many arguments).

To Reproduce

Create an exercise about a function foo : int -> int and test it for instance with Test_lib.test_function_1 with at least one negative test case.

Expected behavior

An output such as : Computing foo (-6) ... (with parenthesis around the negative numbers)

Additional context

I've looked a bit at learnocaml source code, it looks like this issue is due to Introspection.print_value being too optimistic in its need_parentheses sub-routine. I'd be tempted to add - to the characters triggering the case `Decided true (around line 186 in introspection.ml).

@erikmd erikmd self-assigned this Oct 13, 2021
@erikmd erikmd added this to the learn-ocaml 0.13.1 milestone Oct 13, 2021
erikmd added a commit to pfitaxel/learn-ocaml that referenced this issue Oct 13, 2021
erikmd added a commit to pfitaxel/learn-ocaml that referenced this issue Oct 13, 2021
@erikmd
Copy link
Member

erikmd commented Oct 13, 2021

Thanks a lot @letouzey for your very detailed bug report!

I can reproduce it; I've opened a PR adding a test + your suggested fix,
but the CI does not passes yet, so I'll have another look ASAP.

erikmd added a commit to pfitaxel/learn-ocaml that referenced this issue Oct 15, 2021
…l-sf#440

* Thanks @letouzey for reporting this issue and suggesting a fix

* Update some learn-ocaml-client tests accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants