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

Integrate learn-ocaml-editor #295

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

Conversation

erikmd
Copy link
Member

@erikmd erikmd commented Jul 5, 2019

Kind: feature
Depends: #302

Dear all,

This PR adds the "learn-ocaml-editor" feature that has been developed in https://github.com/pfitaxel/learn-ocaml-editor by the PFITAXEL team, recently migrated by @leunam217 to the latest version of learn-ocaml (18f0ddc) then simplified − removing one view (high-level version of "Test") and renaming "Test.ml" to "Test" − to anticipate an upcoming evolution to be implemented within @leunam217's internship co-supervised by @yurug and me.

Brief overview of the design choices

From the beginning of the development of this feature, the idea is to provide a local web environment (fully client-side & statically served) for the teachers to create learn-ocaml exercises with:

  • a facility to edit all the components of an exercise in the "exodir format" (viz. solution.ml, template.ml, prepare.ml, prelude.ml, test.ml, metadata etc.) with a faster development cycle for teachers.
  • in particular:
    • some "Check" buttons are available in all the corresponding tabs
      (albeit since the migration an issue has to be fixed with the "Test" tab) [now fixed]
    • a "Generate" feature allows one to create monomorphic test cases automatically from the solution, as well as a "Quality Code" section that we had devised for our teaching unit "PFITA"
    • a "Grade" feature allows one to grade the solution w.r.t. the test suite
    • and an "Experiment" feature (which has been temporarily disabled due to migration concerns) that will allow teachers to immediately visualize the devised exercise from a student point of view (in particular, learn-ocaml's original features "Check" and "Grade" will be available in this view)

There is currently a JSON import/export feature for exercises, but we plan to work after this PR on enhancing these import/export facilities (e.g. targetting .zip files in the exodir format and/or fetch from external Git exercises repos)

There is now a .zip import/export feature for exercises in the exodir format.
(but we could implement later on another import facility, e.g. to fetch from external Git exercises repos)

Summary of the changes brought by this PR

  • Add the button Editor in the main view of the web app
    (Note: this button is only shown when the token is a teacher one, but the Editor URL itself is not protected (this seems unnecessary) because by design the editor is local-only and doesn't allow inspecting the compiled exercices: it is intended to create new exercices and possibly later fetch from external Git repositories some exercises to be edited)
  • Change Learnocaml_data to add data-types used by the editor
  • Add the folder editor containing the corresponding files
    (Note: this can be refactored further, especially editor.ml which is strongly inspired by learnocaml_exercise_main.ml)
  • Do a few changes in toplevel (including adding an optional arg to Learnocaml_toplevel.check) to implement the "Check" and "Generate" features for the "Test" tab, while reusing learn-ocaml's codebase.
  • Extend the local storage code to save the teacher's exercises that are edited locally.

Overview of the interface

Below is a screenshot [updated] to give a sketch of the corresponding interface:

2019-07-23_14-30-12_Screenshot_learn-ocaml-editor+

FYI a statically deployed version (using Github pages) of the previous version of learn-ocaml-editor (0.4.0) is available here: https://pfitaxel.github.io/pfitaxel-demo/

(we'll be able to deploy the latest version of "learn-ocaml+editor" − corresponding to this PR − statically as well if #288 is addressed)

Things to be done before merging this PR

  • Finish the integration of the "Experiment" feature
  • Investigate the remaining issue with the "Check" feature of the "Test" tab
  • Add a feature to warn (and propose to save the exercise) in case of browser tab close or refresh
  • Implement some .zip import/export feature in the exodir format
  • Refactor the Test/Generate feature to regroup questions about the same function (@erikmd)
  • Resolve some remaining little bugs (@leunam217)
  • Update the "Generate" feature when Extend ppx_metaquot with an extension point [%funty: _ -> _] #302 is merged (@erikmd)
  • (2022-01-07) 1. fix the blocking bug I had isolated in this commit: a06f37e
    (I can say I'm up for any help on this issue anytime soon 😅 Cc @yurug @AltGr)
  • (2022-01-07) 2. finish the refactoring/polishing of (ocaml) code I undertook, fixing along the way a few other minor bugs I have spotted
  • (2022-01-07) 3. squash the commits in a better release-please compliant way (but we may probably want to keep the commits in an archived branch, for historical purpose)

erikmd and others added 17 commits June 24, 2019 01:19
* Rebased on 8ba4e3a

Co-authored-by: Manuel Cabarcos-Baulina <[email protected]>
Co-authored-by: Romain Grimal <[email protected]>
Co-authored-by: Damien Guagno <[email protected]>
Co-authored-by: Alexandre Perge <[email protected]>
Co-authored-by: Sophie Rumin <[email protected]>
Co-authored-by: Manuel Cabarcos-Baulina <[email protected]>
Co-authored-by: Romain Grimal <[email protected]>
Co-authored-by: Alexandre Perge <[email protected]>
Co-authored-by: Sophie Rumin <[email protected]>
* Switch to opam 2
* Compile (_: test_qst_untyped) directly to Test_lib
* Disable compilation of TestAgainstSpec for now
* Replace src/grader/test_lib.{ml,mli} with
  ocaml-sf/learn-ocaml@46ded0b
* Start improving the polymorph_detector

Co-authored-by: Manuel Cabarcos-Baulina <[email protected]>
Co-authored-by: Romain Grimal <[email protected]>
* Refactor strings using {| ... |}
* Fix order bug if generating/compiling a testhaut with 10+ questions
  + Refactor src/state, src/editor to replace most StringMap with IntMap
  + Remove fetch_test_index that seems unnecessary
* Don't ask confirm for individual delete of testhaut question
* Reimplementation of the Generate feature using Js Regexp, not (char list)s
* Put the code quality section at the beginning
* Replace "Dummy_Params" with a shorter identifier "Mock"
* Fix Check_Test.ml to take {Prelude,Prepare} contents into account
* Improve Check_Test.ml: Auto-select {Prelude,Prepare} tab (and line) if error
Fix the conflicts of the migration, leading to the following changes:
* Remove the "testhaut" view to have a stronger focus on the
  underlying OCaml code for "test.ml";
* New implementation of "Generate" that bypasses the "testhaut" view.

The application smoothly compiles and runs, but the following tasks remain:
* The "Experiment" feature of the editor is temporarily commented-out;
* There is a remaining isssue with the "Check" feature of the "Test" tab;
* Internationalization is still to be done.
* Adapt learnocaml_toplevel_worker_main.ml to conditionally load
  Embedded_grading_cmis.
* If error is at line No. < 0, switch first from "Test" to "Prepare".
erikmd added 11 commits July 7, 2021 14:06
if (one creates a new exercise in the editor and) adds a solution+test
then click on Check and very quickly, click on Save&Grade!

Now, the "Check" button disables "Save&Grade!" and conversely.
* Fix static deployment of learn-ocaml-editor
* otherwise the interface is quite unpractical, e.g., using ext_alert
  with custom buttons would need to copy in the client code, the
  definition of box_button; so, DRY!
* Add some (commented-out) debug code facilities;
* Add some TODO/FIXME/XXX remarks;
* Workaround the issue by ignoring the 'exercise.descr' string in editor_lib.ml
  (otherwise its encoding-then-*decoding* fails)
@erikmd erikmd added the kind: feature New user-facing feature. label Jul 23, 2021
* upstream/master: (37 commits)

Conflicts:
  learn-ocaml.opam
It is enabled by default:

  $ learn-ocaml build
    <=>
  $ learn-ocaml build --enable-editor

unless we run the command line:

  $ learn-ocaml build --disable-editor
* In sub-directories:

  - static/editor/ (*.html)
  - _opam/share/learn-ocaml/editor/ (*.js|*.html)

* Rename one resource as well (s/new_exercise/new-exercise/)

* Ensure `learn-ocaml build --disable-editor` does not copy files into www.
To be more precise (using Bash syntax for more clarity):

* Beforehand,

    we used "q_${name}" and "q_${name}_1", "q_${name}_2"

    which could be problematic if some ${name} ends in "_1".

* Henceforth,

    we use "q_${name}" and "q1_${name}", "q2_${name}".
(albeit learn-ocaml-editor might be internationalized at some point)
Regarding optional e-mails, for the sake of consistency, use the same
as that of PR ocaml-sf#362 (branch oauth-moodle-dev).

& Do some minor HTML cleanup as well.
@yurug
Copy link
Collaborator

yurug commented Jan 3, 2022

Hello @erikmd : what's the status of this PR? Should we close it so that you can integrate its feature in a more incremental way?

@erikmd
Copy link
Member Author

erikmd commented Jan 7, 2022

Hi @yurug, thanks anew, so the feature of this branch is almost ready but there are three things that remain to do:

    1. fix the blocking bug I had isolated in this commit: a06f37e
      (I can say I'm up for any help on this issue anytime soon 😅 Cc @yurug @AltGr)
    1. finish the refactoring/polishing of (ocaml) code I undertook, fixing along the way a few other minor bugs I have spotted
    1. squash the commits in a better release-please compliant way (but we may probably want to keep the commits in an archived branch, for historical purpose)

but no need to close the PR I guess, I'll be able to force-push once the branch has been archived.

Edit: I added these three steps at the end of the description of the issue

@erikmd
Copy link
Member Author

erikmd commented Jan 7, 2022

To be a bit more specific of the blocking issue partly documented in commit a06f37e → the fact is that in (Learn-OCaml)Editor, tested with Firefox, when we click on Experiment to switch from teacher view to student view (and load the exercise data from the Editor tabs), if the descr.md text is not empty but has e.g. ≥14000 chars, then we get a JS Stack_overflow :-/

Furthermore, this issue was not "present" at all in the initial version of (Learn-OCaml)Editor… (I had done a bisection and noticed the issue was introduced when the src/repo/learnocaml_exercise.ml datatypes had been extended in learn-ocaml master… but I don't remember the exact PR number I had found, sorry for that)

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.

4 participants