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.

Sign up for GitHub

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 constructors for values of type Omd.doc #268

Merged
merged 6 commits into from
Aug 1, 2022
Merged

add constructors for values of type Omd.doc #268

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7259e32
add constructors for values of type `Omd.doc`
tatchi May 3, 2022
f913b59
remove attributes for {soft,hard}_break and hr
tatchi May 13, 2022
ab6c48d
fmt
tatchi May 13, 2022
40a3cad
update names
tatchi Jun 1, 2022
759c364
Merge branch 'master' into add-constructors
shonfeder Jul 24, 2022
1db13d5
Merge branch 'master' into add-constructors
shonfeder Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/omd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,26 @@ let parse_inlines (md, defs) =
in
List.map (Mapper.map (parse_inline defs)) md

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 dl for def_list and code_bl for code_block?

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 🤗

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks so much for the PR and sorry for the very loooong turn around in merging this in. :)

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)
Expand Down
28 changes: 28 additions & 0 deletions src/omd.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,34 @@ type 'attr block =
type doc = attributes block list
(** A markdown document *)

val txt : ?attrs:attributes -> string -> attributes inline
val em : ?attrs:attributes -> attributes inline -> attributes inline
val strong : ?attrs:attributes -> attributes inline -> attributes inline
val code : ?attrs:attributes -> string -> attributes inline
val hard_break : attributes inline
val soft_break : attributes inline
val link : ?attrs:attributes -> attributes link -> attributes inline
val img : ?attrs:attributes -> attributes link -> attributes inline
val html : ?attrs:attributes -> string -> attributes inline
val pg : ?attrs:attributes -> attributes inline -> attributes block

val ul :
?attrs:attributes
-> ?spacing:list_spacing
-> attributes block list list
-> attributes block

val ol :
?attrs:attributes
-> ?spacing:list_spacing
-> attributes block list list
-> attributes block

val blq : ?attrs:attributes -> attributes block list -> attributes block
val hr : attributes block
val code_block : ?attrs:attributes -> label:string -> string -> attributes block
val html_block : ?attrs:attributes -> string -> attributes block
val def_list : ?attrs:attributes -> attributes def_elt list -> attributes block
val of_channel : in_channel -> doc
val of_string : string -> doc
val to_html : doc -> string
Expand Down