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

Implement multi-part exercises #402

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

YoanwM
Copy link
Contributor

@YoanwM YoanwM commented Jun 23, 2021

This PR implements the feature requests #331 and #395.

(Direct link to the specification of subindex.json)

@erikmd

This comment has been minimized.

@@ -711,7 +920,7 @@ module Exercise = struct
module Index = struct

type t =
| Exercises of (id * Meta.t option) list
| Exercises of (id * Meta.t option * Subindex.t option) list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the Subindex.t type already contains a Meta.t, I'd suggest refactoring this type into 3 constructors
(to avoid by construction the "case that can't occur" :)
Hoping this small refactoring won't have any impact on the rest!

@@ -171,6 +171,8 @@ module Exercise: sig

val enc: t Json_encoding.encoding

val sub_enc: t Json_encoding.encoding
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in our meeting, you should be able to remove that line (enforcing that there is a learnocaml_version: "2" in subindex.json? - I've just updated #331 (comment))

Comment on lines +257 to +262
module Subindex = struct
include Exercise.Subindex

(*let get_from_subindex subindex =
Exercise.Subindex*)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused code? (to be removed in that case)

@@ -21,24 +21,49 @@ type t =
dependencies : string list;
}


type t =
| Subexercise of (id * exercise list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a reminder: this type should be refined to include the boolean student_hidden and the weights (student_weight, teacher_weight)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as well as the boolean check_all_against)

Comment on lines 81 to 84
Lwt_list.map_p
(fun exo -> Grader_cli.grade ?print_result ?dirname meta
(Learnocaml_exercise.Exercise exo) output_json)
exs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and just to recap, here you'll probably implement the check_all_against feature
let's say that if you have [ex1; ex2; ex3; exall] with "check_all_against": "exall", you'd grade
[ex1; ex1_with_exall_tests; ex2; ex2_with_exall_tests; ex3; ex3_with_exall_tests; exall].
Does this sound OK for you @YoanwM ?

@@ -143,7 +143,7 @@ val stars_div: float -> [> Html_types.div ] Tyxml_js.Html5.elt

(** Returns an HTML string expected to be put in an iframe *)
val exercise_text:
Exercise.Meta.t -> Exercise.t -> string
Exercise.Meta.t -> Learnocaml_exercise.t -> string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this legit? (I'm not saying this is not the case 🙂 but we'll just need to have a look)

@YoanwM YoanwM marked this pull request as draft June 23, 2021 14:21
@erikmd erikmd changed the title Learnocaml v3 Implement multi-part exercises Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants