add constructors for values of type Omd.doc
#268
Conversation
There was a problem hiding this comment.
A warm welcome and many thanks for the contributions, @tatchi! I'm very glad you're pitching to help here 😄
LGTM, modulo one suggestion about elements that can't (afak) meaningfully do anything with attributes at this level.
Let me know what you think!
src/omd.ml
Outdated
| let txt ?(attrs = []) s = Text (attrs, s) | ||
| let em ?(attrs = []) il = Emph (attrs, il) | ||
| let strong ?(attrs = []) il = Strong (attrs, il) | ||
| let code ?(attrs = []) s = Code (attrs, s) | ||
| let hard_break = Hard_break [] | ||
| let soft_break = Soft_break [] | ||
| let link ?(attrs = []) li = Link (attrs, li) | ||
| let img ?(attrs = []) li = Image (attrs, li) | ||
| let html ?(attrs = []) s = Html (attrs, s) | ||
| let pg ?(attrs = []) il = Paragraph (attrs, il) | ||
| let ul ?(attrs = []) ?(spacing = Loose) l = List (attrs, Bullet '-', spacing, l) | ||
|
|
||
| let ol ?(attrs = []) ?(spacing = Loose) l = | ||
| List (attrs, Ordered (1, '.'), spacing, l) | ||
|
|
||
| let blq ?(attrs = []) blocks = Blockquote (attrs, blocks) | ||
| let hr = Thematic_break [] | ||
| let code_block ?(attrs = []) ~label s = Code_block (attrs, label, s) | ||
| let html_block ?(attrs = []) s = Html_block (attrs, s) | ||
| let def_list ?(attrs = []) l = Definition_list (attrs, l) |
There was a problem hiding this comment.
Looking at this more closely now, I think we should come up with a clear rationale for how we name these functions. Note that I see these names are taken from my initial sketch in the originating issue, so this is me reconsidering my initial half backed suggestion :)
Some of these are named after the HTMl entities, some are named after the value constructor of the AST's sum types, and some (namely blq and def_list) have abbreviations I don't associate with any standard source.
I see three options:
- The easiest thing would be just to name all the functions exactly the same as the constructors.
- A very invasive approach, but one that could perhaps move us towards an optimal API, is to find a better set of names (by some criteria: More concise? More standard?) and name these function and the value constructors all the same.
- Lastly, we could accept a divergence of names between the sum type constructors and the functions, but we should still probably come up with some story to explain why we chose what for which functions. So then maybe we should optimize for short names? Like
dlfordef_listandcode_blforcode_block?
What do you think?
There was a problem hiding this comment.
I think I like to have short names for the function constructors. Option 2 is quite invasive so I would go for option 3.
When possible, we can try to match the Tyxml API? We can take some inspiration from: https://github.com/ocaml/omd/pull/211/files#diff-16f58dc281193a8336f88df2eeb14760638cfc1ce4a2a1c479c509e33f39738f
And for the rest, well, we can try to do our best 😅
That would give something like:
diff --git a/src/omd.ml b/src/omd.ml
index 012604c..8922228 100644
--- a/src/omd.ml
+++ b/src/omd.ml
@@ -18,22 +18,22 @@ let txt ?(attrs = []) s = Text (attrs, s)
let em ?(attrs = []) il = Emph (attrs, il)
let strong ?(attrs = []) il = Strong (attrs, il)
let code ?(attrs = []) s = Code (attrs, s)
-let hard_break = Hard_break []
-let soft_break = Soft_break []
-let link ?(attrs = []) li = Link (attrs, li)
+let hard_br = Hard_break []
+let soft_br = Soft_break []
+let a ?(attrs = []) li = Link (attrs, li)
let img ?(attrs = []) li = Image (attrs, li)
let html ?(attrs = []) s = Html (attrs, s)
-let pg ?(attrs = []) il = Paragraph (attrs, il)
+let p ?(attrs = []) il = Paragraph (attrs, il)
let ul ?(attrs = []) ?(spacing = Loose) l = List (attrs, Bullet '-', spacing, l)
let ol ?(attrs = []) ?(spacing = Loose) l =
List (attrs, Ordered (1, '.'), spacing, l)
-let blq ?(attrs = []) blocks = Blockquote (attrs, blocks)
+let blockquote ?(attrs = []) blocks = Blockquote (attrs, blocks)
let hr = Thematic_break []
-let code_block ?(attrs = []) ~label s = Code_block (attrs, label, s)
-let html_block ?(attrs = []) s = Html_block (attrs, s)
-let def_list ?(attrs = []) l = Definition_list (attrs, l)
+let code_bl ?(attrs = []) ~label s = Code_block (attrs, label, s)
+let html_bl ?(attrs = []) s = Html_block (attrs, s)
+let dl ?(attrs = []) l = Definition_list (attrs, l)
let of_channel ic = parse_inlines (Pre.of_channel ic)
let of_string s = parse_inlines (Pre.of_string s)
let to_html doc = Html.to_string (Html.of_doc doc)blockquote feels a bit different to me since that's the only one that isn't abbreviated.
There was a problem hiding this comment.
Good thinking to look to TyXml!
I think blockquote is ok, despite the disparity, since it matches the html tag and the TyXml constructor.
These generally look ok to me. My only point of concern is around the a function. The markdown links seems to not really map exactly onto to general html anchors, but rather be a special case of them. But maybe I am over thinking that a bit.
Lastly, since we are not tending towards the side of making the function names match the value constructors perfectly, what would you think about this for the breaks?
-let hard_break = Hard_break []
-let soft_break = Soft_break []
+let br = Hard_break []
+let nl = Soft_break []I think this matches the semantics of these things fairly well, and br fits the TyXml and HTML I think?
WDYT?
There was a problem hiding this comment.
I think this matches the semantics of these things fairly well, and br fits the TyXml and HTML I think?
WDYT?
That makes sense to me :)
I updated the code, thanks for your suggestions 🤗
There was a problem hiding this comment.
Thanks so much for the PR and sorry for the very loooong turn around in merging this in. :)
|
I've reported the dependency build failure blocking ci here upstream: ocaml/camlp-streams#7 |
Fixes: #252
Not sure if all the constructors make sense. Open to suggestions, also regarding the functions' names.