AST representation changes

[sonologico]

Opening the PR to continue the discussion started on #228.

Changes to the AST are:

• Remove the record wrapper for each node and inline the attribute field
• Make the block type parametric with the goal of using this flexibility to later have a way to parse and return the AST with source locations and whatever information we might want per node.

[shonfeder]

I left a few suggestions, but they are just that. Let me know if you think any of them aren't for the better :)

I'm psyched to get this in place!

[shonfeder]

It's a bit strange at first to see these constructors take attributes, tho we have a good reason. Perhaps a comment on these constructors (or above this type declaration) to explain why we want to old space for attrs throughout would be helpful for future readers?

[sonologico]

That's a good point. Haven't added a comment yet. Not sure if I want to add a comment here or at the "external interface" level (omd.mli). I'm leaning towards adding it here. I'll add something before we merge this.

[sonologico]

I have added a comment. Let me know if it's good enough.

[shonfeder]

Just an idea for consideration -- one reason we might think about nested modules here is for namespacing like:

module Raw = struct
  type inline = string
  type t = (inline, attributes) block
end

module Plain = struct
  type inline = attributes inline
  type t = (inline, attributes) block
end

module SourceMap = struct
  type loc
  type inline = loc inline
  type t = (inline, loc) block
end

We might also end up with some functors that would apply to any of these ast representations. That might be too speculative, and perhaps not worth the complication currently, but just wanted to float the idea.

[sonologico]

I can see the benefit of it. I don't feel strongly about it, but I would either

• keep it as is and wait until we have a need for functors (which is what I'd favour)
• do it in the previous style in which the constructors are part of a functor so that they are also namespaced

[sonologico]

Let me know what you think after I have changed the rest of the code.

If we're going with this, I think that removing the open Ast line in some files and using the full path throughout the code would be better. What do you think?

[shonfeder]

Looking good to me! Just two small comments.

After having propagated the AST through the code base, what's your feeling on its merits compared to the current representation? Do you feel like it's an improvement to work with?

[shonfeder]

How about exposing the Ast.t in this interface and then using it here?

Suggested change

	type doc = (attributes, attributes inline) block list
	type doc = attributes Ast.t list

[sonologico]

This is the thing that is making me consider moving back to using a functor as we were using before in the Ast module. There's no way to expose block without a parameter here. It feels weird to have Omd.block, Omd.t and Omd.doc.

I'd like to either

1. Pick a different name for block and use the name block for t and update the Ast module to match it
2. Go back to having the functor in the Ast module as it was before so that block doesn't have a type parameter and we don't need a t.

Let me know what you think and I'll try to wrap up this up by tomorrow so that we can unblock the other things.

[shonfeder]

I'm afraid I don't quite follow. I think it makes sense to have a these two different type constructors exposed:

• ('attr, 'a) block: types for block-level elements of any attributes and any content
• 'attr Ast.t: types for the standard markdown AST, over any attributes

And to have this additional type:

• doc = attributes t list: the type for the standard representation of a markdown document, with the standard attributes.

Tho I could also see an argument Ast.t being defines as a list of blocks.
(Well, if I were designing from scratch, I'd probably call doc Omd.t, but that's inessential.)

That said, if you think some use of simple functors will improve the interface and make a nicer presentation of types, I wouldn't be opposed. Do you want to sketch out what you have in mind?

[sonologico]

The Ast module used a functor. I'm thinking of reintroducing that so that Omd can have the following signature:

(* ...  *)

type 'attr inline =
  | Concat of 'attr * 'attr inline list
  | Text of 'attr * string
  | Emph of 'attr * 'attr inline
  | Strong of 'attr * 'attr inline
  | Code of 'attr * string
  | Hard_break of 'attr
  | Soft_break of 'attr
  | Link of 'attr * 'attr inline link
  | Image of 'attr * 'attr inline link
  | Html of 'attr * string

(* ... *)

type 'attr block =
  | Paragraph of 'attr * 'attr inline
  | List of 'attr * list_type * list_spacing * 'attr block list list
  | Blockquote of 'attr * 'attr block list
  | Thematic_break of 'attr
  | Heading of 'attr * int * 'attr inline
  | Code_block of 'attr * string * string
  | Html_block of 'attr * string
  | Definition_list of 'attr * 'attr def_elt list

type doc = attributes block list

With the current setup in my branch, we can't really expose Ast.t in Omd without exposing the Ast module as well (maybe with a restricted signature). I find that this Omd signature, the solution currently in my branch, is awkward:

(* ... *)
type ('attr, 'a) block =
  | Paragraph of 'attr * 'a
  | List of 'attr * list_type * list_spacing * ('attr, 'a) block list list
  | Blockquote of 'attr * ('attr, 'a) block list
  | Thematic_break of 'attr
  | Heading of 'attr * int * 'a
  | Code_block of 'attr * string * string
  | Html_block of 'attr * string
  | Definition_list of 'attr * 'a def_elt list

type 'attr t = ('attr, 'attr inline) block (* equivalent to Ast.t *)

type doc = attributes t list
(** A markdown document *)

I would either change the name of block to something else and change t to block or change t to something else.

[sonologico]

Added back the functor to the Ast module. There's no real cons internally besides a heavier weight syntax. I prefer the simpler external interface with a single parameter for the attributes on the block and inline types in Omd.

If you give me your approval, I'll squash the commits and merge this.

[shonfeder]

If we're going with this, I think that removing the open Ast line in some files and using the full path throughout the code would be better. What do you think?

I'm fine with narrower scoping of the module if you think it's an improvement 👍

[sonologico]

Looking good to me! Just two small comments.

After having propagated the AST through the code base, what's your feeling on its merits compared to the current representation? Do you feel like it's an improvement to work with?

I think it's an improvement overall.

• I like having just the variants without the wrapping records. The small downside is that we might want to add a function to pattern match and return the attributes uniformly. I think it's worth it.
• Adding the type parameter for the attributes has made types a bit noisier. I think it's worth it, because it hasn't made the type harder to work with and it makes adding support for an alternative more detailed representation (with locations, etc) easier.

[shonfeder]

I'm not crazy about the functorial code here, but that's inherited :)

I suspect we can maybe find a nicer way of wrangling these types in the future, but I think the external API exposed in terms of the AST won't change!

LGTM!