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

Add support for Markdown in exercises #135

Merged
merged 2 commits into from
Sep 14, 2018
Merged

Add support for Markdown in exercises #135

merged 2 commits into from
Sep 14, 2018

Conversation

Niols
Copy link
Contributor

@Niols Niols commented Sep 12, 2018

This PR is a small modification of the learnocaml_exercise.ml file that allows for support for Markdown in exercises description. It modifies the functions read_descr and read_descrs in such a way that:

  • read_descr now has a new argument that is the list of extensions to try in order. I also fixed a bug that whould have happened if land was not empty in read_descrs.

  • read_descrs now has a list of extensions to try that it can give to read_descr. I believe this list should at some point move somewhere else. Maybe in the descr record?

I had to add a small markdown_to_html function. I used the basic functions from Omd, which means that it might not support other features that you want to have in your flavour of Markdown. I believe there should be a module (Markdown?) that implements this flavour with an interface containing at least from_string and to_html.

let key =
Filename.remove_extension descr.key
^ "." ^ lang ^ "." ^
Filename.extension descr.key in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filename.extension already contains the dot. This is fixed in this PR. If the PR is not merged, this should be fixed anyway.

return ()
in
let markdown_to_html md =
Omd.(md |> of_string |> to_html)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function should probably go in a module that implements learn-ocaml's flavour of Markdown.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Do it if you want.

let exts = [
(Filename.extension descr.key, fun h -> h) ;
(".md", markdown_to_html)
] in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These lists should probably move at some point. Maybe the optionnal extensions should go in the descr record?

@yurug yurug requested a review from picdc September 12, 2018 12:47
@yurug
Copy link
Collaborator

yurug commented Sep 12, 2018

This PR is fine to me. @OCamlPro-Couderc I let you review it since you are the expert of this piece of code.

@AltGr
Copy link
Collaborator

AltGr commented Sep 12, 2018

This is great, thanks! Did'nt have time to get around to it yet :)
It would be best to plug in the markdown stuff that we use for the tutorials: it has, for example, support for mathjax snippets. I don't know how easy it would be, though.

@picdc
Copy link
Collaborator

picdc commented Sep 13, 2018

This PR seems nice to me.
I was just thinking at one thing: we use "descr.html" as the key to represent the value descr. However, with the addition of internationalized description files and the support for markdown, this key is no longer relevant. It works for now since we convert everything into HTML and with one language by instance, but that's something we could improve in the future.

@Niols
Copy link
Contributor Author

Niols commented Sep 13, 2018

It would be best to plug in the markdown stuff that we use for the tutorials: it has, for example, support for mathjax snippets. I don't know how easy it would be, though.

@AltGr I do agree. There are a lot of things that could be extracted from https://github.com/ocaml-sf/learn-ocaml/blob/master/src/repo/learnocaml_tutorial_parser.ml. But the parse_tutorial functions are quite long and I don't pretend to understand exactly what everything does. It has to do with the Tutorial.text type found in https://github.com/ocaml-sf/learn-ocaml/blob/master/src/state/learnocaml_data.ml. This type and the functions converting from and to HTML and Markdown should probably have their own separate file (src/utils/richText.ml?).

I was just thinking at one thing: we use "descr.html" as the key to represent the value descr. However, with the addition of internationalized description files and the support for markdown, this key is no longer relevant. It works for now since we convert everything into HTML and with one language by instance, but that's something we could improve in the future.

@OCamlPro-Couderc I do agree too. The type 'a file is not generic enough to handle the descr file. I'm not sure if one should generalize it or agree that descr is a particular file.

@yurug yurug merged commit 6b4c35a into ocaml-sf:master Sep 14, 2018
@Niols
Copy link
Contributor Author

Niols commented Sep 14, 2018

There was a problem with the merge. It's because the PR was anterior to the "dunification" of learn-ocaml. And the Omd dependency lacks in this PR. One has to change https://github.com/ocaml-sf/learn-ocaml/blob/master/src/repo/dune#L6 and add "omd" as a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants