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

Support structure definitions over multiple code fences #158

Open
craigfe opened this issue Aug 8, 2019 · 11 comments
Open

Support structure definitions over multiple code fences #158

craigfe opened this issue Aug 8, 2019 · 11 comments
Labels
Kind/Feature-request New feature or request

Comments

@craigfe
Copy link
Contributor

craigfe commented Aug 8, 2019

The Irmin tutorials currently contain many instances of Markdown text within a module definition, e.g:

```ocaml
module Counter: Irmin.Contents.S with type t = int64 = struct
	type t = int64
	let t = Irmin.Type.int64
\```

Now we need to define a merge function:

\```ocaml
	let merge ~old a b =
	    let open Irmin.Merge.Infix in
		old () >|=* fun old ->
        let old = match old with None -> 0L | Some o -> o in
        let (+) = Int64.add and (-) = Int64.sub in
        a + b - old

    let merge = Irmin.Merge.(option (v t merge))
end
\```

This is particularly important when tutorialising backend definitions, as these require several large module definitions. However, it appears that these are currently unsupported by ocaml-mdx:

ᐅ ocaml-mdx test data/tutorial/contents.md
Got an error while evaluating:
---
module Counter: Irmin.Contents.S with type t = int64 = struct
	type t = int64
	let t = Irmin.Type.int64
---
Characters 110-110:
Error: Syntax error: 'end' expected
Characters 55-61:
Error: This 'struct' might be unmatched

It would be nice if mdx would support this sort of pattern 🙂

@Julow
Copy link
Collaborator

Julow commented Sep 9, 2019

To fix that, you can write the snippets to a file, using the file=... and part=... labels and then compile it yourself. This is not ideal.

Maybe related to realworldocaml/book#3150 where code is written to a file but not executed.

@NathanReb
Copy link
Contributor

A work around that we use for these kind of examples in RWO is to extract the code fragments from actual example .ml files that we (will soon) build separately in the CI to ensure they are correct. You can do that by using file=path/to/example.ml and part=x labels on your block.

That works because in this case, mdx doesn't try to evaluate the block.

@NathanReb
Copy link
Contributor

1 second too slow, damn...

@craigfe
Copy link
Contributor Author

craigfe commented Sep 9, 2019

We're currently just using ocaml-mdx pp to write everything to a file, then checking that the file compiles. I prefer this because it doesn't require explicitly passing labels to each block (in particular, this label syntax does not play nicely with the markdown parser used to generate the site).

https://github.com/CraigFe/irmin.io/blob/mdx-verification/data/tutorial/dune

@gpetiot gpetiot added the Kind/Feature-request New feature or request label Mar 10, 2020
@gpetiot
Copy link
Contributor

gpetiot commented Mar 12, 2020

Would #205/#234 be enough to have you adopt the workflow of using file/part labels?

@craigfe
Copy link
Contributor Author

craigfe commented Mar 12, 2020

It's not ideal, since (as far as I understand) it requires one of:

  • adding mdx (and all of its dependencies) to the build step for the website;
  • setting up a diff-promote workflow and committing redundant build artefacts.

What I have now is a literate programming workflow in which .md files can be exported to valid .ml for compilation / testing. This has three advantages and one disadvantage:

  • +ve. Changing a page touches only a single file. Authors see the code snippets inline in the prose description, exactly as it will be rendered, and there's no switching back and forth between two files and trying to keep them in sync.

  • +ve. The code written in the .md file is exactly the code that is tested against, in the same order (unless explicitly escaped with skip). With the sync workflow this becomes a constraint that the authors have to maintain.

  • +ve. Doing it in this direction means that mdx can be in the same switch used for running the output, avoiding the need for committing build artefacts or adding mdx to the deploy toolchain.

  • -ve. I can't run OCamlformat on code snippets in .md files.

However, it seems fine to me if Mdx maintainers decide that a literate programming workflow is out-of-scope and would be better filled by other (new) tools: e.g. Coqdoc for OCaml or literate Haskell for OCaml (both of these require an explicit 'render as Markdown' step, but have the advantages of also being renderable as e.g. LaTeX).

For now, the pp workflow seems as close as it's possible to get.

@NathanReb
Copy link
Contributor

+ve. Doing it in this direction means that mdx can be in the same switch used for running the output, avoiding the need for committing build artefacts or adding mdx to the deploy toolchain.

I'm not sure I understand this specific point.

The whole point of #205 and #234 is to make mdx files compatible with external markdown tools so that you don't need mdx in your deploy toolchain.

I understand that you don't want to maintain two files (one for the code and one for the doc) and this is a perfectly valid reason not to use "regular" mdx here.

I'm not sure the other points are valid though. In particular you currently have to maintain a set of rules for properly invoking mdx pp which seems like a lot more work than adding something like this to your dune file:

(library
  (name <example_module>)
  ...)

(alias
 (name runtest)
  (deps (alias check))

The new mdx stanza is fairly light and keeping the example ml file and the doc in sync is just a matter of running dune runtest so I would hardly consider it an effort.

I'm not trying to convince you that you should change your workflow but rather to better understand why the regular one doesn't currently suit you and/or what a litterate programming tool should look like.

@craigfe
Copy link
Contributor Author

craigfe commented Mar 12, 2020

I'm not sure I understand this specific point.

The whole point of #205 and #234 is to make mdx files compatible with external markdown tools so that you don't need mdx in your deploy toolchain.

I agree in as much as it makes e.g. the skip label compatible with a non-mdx toolchain, but I don't think it solves the problem with the sync workflow (that is, file and part labels) that they require either committing the sync targets or generating them pre-deploy.

I'm not sure the other points are valid though. In particular you currently have to maintain a set of rules for properly invoking mdx pp which seems like a lot more work than adding something like this to your dune file:

(library
  (name <example_module>)
  ...)

(alias
 (name runtest)
  (deps (alias check))

Hmm, isn't this a new point altogether? I didn't mention the dune rules at all in my explanation. In practice, there isn't much maintenance work for our mdx pp rule now that I've written it. The big maintenance burden of the sync workflow (as I see it) is that you have to maintain file and part correspondences & ensure that no blocks are skipped or rendered out-of-order etc.

The new mdx stanza is fairly light and keeping the example ml file and the doc in sync is just a matter of running dune runtest so I would hardly consider it an effort.

dune runtest specifically would also run the tests, which isn't something I'd want to do in the standard 'write → render → check' web development loop. That's probably easily avoidable with a different alias.

I'm not sure it's that effortless. Having any kind of manual sync step at all is a bit problematic since it gets in the way of incremental compilation / "development" servers that allow you to work against the rendered output. That's a shame, since it means you can't use such tools out-of-the-box, but again workarounds probably exist. It also makes the opam switch a compulsory part of the development environment, even for small things like typo/grammar fixes that are often done by non-maintainers.

@NathanReb
Copy link
Contributor

Ok I'm starting to understand. The main issue here really is the fact that the .md file isn't the sole source of truth and that you might need part of an OCaml toolchain whenever you edit it to make sure you did not "break" it, am I correct?

I'd argue that the pp approach suffers the same kind of problem as if you edit a code fragment in the md file, you'd still need dune and mdx to make sure the code is still valid. If you edit anything else, then you're fine, no matter which workflow you chose.

Hmm, isn't this a new point altogether?

This was more of an answer about how much of burden it is to setup an example+sync workflow compared to a pp one but I may have misunderstood your point!

@craigfe
Copy link
Contributor Author

craigfe commented Mar 12, 2020

Ok I'm starting to understand. The main issue here really is the fact that the .md file isn't the sole source of truth and that you might need part of an OCaml toolchain whenever you edit it to make sure you did not "break" it, am I correct?

I think the main issue with the .md file not being the sole sense of truth is that authors need to work over two files that don't read sequentially.

I'd argue that the pp approach suffers the same kind of problem as if you edit a code fragment in the md file, you'd still need dune and mdx to make sure the code is still valid. If you edit anything else, then you're fine, no matter which workflow you chose.

This seems to be me just misunderstanding what a "sync workflow" is exactly. I thought there were three files: code.ml + presync.mdpostsync.md, so that even non-code changes to presync.md would require invoking mdx to propagate those changes to postsync.md. I thought this because the README shows file blocks as blank (https://github.com/realworldocaml/mdx#file-sync 🤔), and also because that's how it's done in the expect test suite.

A glance at RWO suggests that that isn't the case, and that mdx can be used to update a single .md file in place, which is a much better workflow 🙂 That probably clears up what I was saying about 'committing build artefacts'...

Given that, I think my concerns with the sync workflow reduce to annoyances with working over more than one file (with implicit, non-sequential correspondences between them).

@NathanReb
Copy link
Contributor

We do really need to work on the documentation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind/Feature-request New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants