automatically fill the id attributes of the headings #267

tatchi · 2022-05-03T06:16:05Z

Fixes: #251

This is my attempt at implementing #251

I didn't add an option to enable/disable that feature. Let me know if I should.

shonfeder

Thanks for the contribution!

I've just one question, but it generally looks solid to me. 🎉

tests/blackbox/heading-id.t

shonfeder

Meant to leave review status as "Comment" :)

shonfeder · 2022-05-13T15:14:00Z

Ah, I just looked at the failures in CI. There are two issues:

To maintain compatibility with OCaml 4.04.2, we'll either need to avoid using the Fun module and write our implementations, or pull in https://github.com/thierry-martinez/stdcompat (or an equivalent, if available). I'm quite happy with doing the latter.
The failures on recent version of OCaml show why this actually does have to be configurable: it breaks all of our specification compliance tests :). I suggest we make this configurable by CLI, by default to having it enabled.

How does that sound?

tatchi · 2022-05-17T05:05:46Z

To maintain compatibility with OCaml 4.04.2, we'll either need to avoid using the Fun module and write our implementations, or pull in https://github.com/thierry-martinez/stdcompat (or an equivalent, if available). I'm quite happy with doing the latter.

I just inlined the function as it was straightforward to do: 0570385

The failures on recent version of OCaml show why this actually does have to be configurable: it breaks all of our specification compliance tests :). I suggest we make this configurable by CLI, by default to having it enabled.

I added an --auto-identifiers option (inspired by pandoc) in cb088d1. Since it's enabled by default it still break the tests. Should I promote them or disable that option when running the tests?

As for the identifiers, there's actually some rules described in the doc on how to derive them:

The default algorithm used to derive the identifier from the heading text is:

Remove all formatting, links, etc.

Remove all footnotes.

Remove all non-alphanumeric characters, except underscores, hyphens, and periods.

Replace all spaces and newlines with hyphens.

Convert all alphabetic characters to lowercase.

Remove everything up to the first letter (identifiers may not begin with a number or punctuation mark).

If nothing is left after this, use the identifier section.

I had to switch to an implementation that supports Unicode characters in eec7388 (using the Uucp and Uutf modules). It's not yet perfect as some of the test results are not correct. I'll continue to try to improve that :)

tatchi · 2022-05-18T05:37:23Z

I improved the algorithm slightly; it should be ok for a second review.

There are still some differences compared to the result I get on https://pandoc.org/try/, but pandoc can only generate id's when using markdown and not commonmark. So the outputted HTML content (from which the id is derived) might be different in some cases, hence why the result might differ.

The only thing missing I think is:

several headings have the same text; in this case, the first will get an identifier as described above; the second will get the same identifier with -1 appended; the third with -2; and so on.

So for instance in pandoc, the following

### hello
### hello

becomes:

<h3 id="hello">hello</h3>
<h3 id="hello-1">hello</h3>

I guess it means that we'll need to keep a state somewhere with all the ids we've already generated. Not sure how to best implement that and if that's something we want. Would love to have your input on that :)

We'll also have to decide how to handle "empty" id. In pandoc:

If nothing is left after this, use the identifier section.

So

### 33
### 22
###

becomes

<h3 id="section">33</h3>
<h3 id="section-1">22</h3>
<h3 id="section-2"></h3>

tmattio · 2022-07-25T15:52:01Z

cc @koonwen, this would probably fix ocaml/ocaml.org#523!

shonfeder · 2022-08-02T23:10:24Z

We'll also have to decide how to handle "empty" id. In pandoc:

I'm fine with following pandoc here.

shonfeder

Looking good! I finally was able to address your outstanding questions. Sorry again for the long delay here!

shonfeder · 2022-08-02T23:09:51Z

src/html.ml

@@ -128,9 +178,13 @@ and inline = function
  | Image (attr, { label; destination; title }) ->
      img label destination title attr

-let rec block = function
+let rec block ~auto_identifiers = function


I guess it means that we'll need to keep a state somewhere with all the ids we've already generated. Not sure how to best implement that and if that's something we want. Would love to have your input on that :)

Perhaps we can have the block function take a record for this argument that can carry some configuration/state? For the identifier numerical suffix, it could be a int StringMap.t. Of course, we could also use a mutable hash map for this, but would be nice to avoid if feasible. WDYT?

Thanks for the suggestion, I added an int StringMap.t to keep track of the numerical suffix. Let me know what you think :)

src/omd.ml

Fixes: ocaml-community#251

Thanks to the index and accumulator arguments given by `Uutf.String.fold_utf_8`, we can avoid the need for a sentinel "start" the need for repeated checks after we've found the suffix, and the need to allocate a new string.

shonfeder

Just a few followup suggestions. I think we're just about there!

tests/blackbox/heading-id.t

src/html.ml

shonfeder · 2022-09-17T15:47:25Z

src/html.ml

+            let id = slugify (to_plain_text text) in
+            (* Default identifier if empty. It matches what pandoc does. *)
+            let id = if id = "" then "section" else id in
+            let count, identifiers = Identifiers.touch id identifiers in
+            let id =
+              if count = 0 then id else Printf.sprintf "%s-%i" id count
+            in


I feel like this might better belong in the slugify function. WDYT?

I don't have any strong opinion on that one. I made the changes in 820f310 (#267)

…for-headings

Similar to the approach with `drop_while`, we can use the accumulator argument of the fold to track whether or not we've seen white space, which allows us to clean up the logic a bit, IMO. However, unlike that case, the need to construct a new string still requires some side effects.

shonfeder

Looks great. Thanks so much, and sorry for the sluggish review loops!

bin/main.ml

shonfeder · 2022-11-20T23:57:20Z

Closing this in favor of #294. I opened up the new branch purely for the purposes of making merge conflict resolution easier.

Thanks so much, @tatchi!

shonfeder approved these changes May 7, 2022

View reviewed changes

tests/blackbox/heading-id.t Outdated Show resolved Hide resolved

shonfeder reviewed May 7, 2022

View reviewed changes

tatchi force-pushed the auto-id-for-headings branch 2 times, most recently from d01fbf0 to eec7388 Compare May 16, 2022 19:01

tatchi force-pushed the auto-id-for-headings branch from c1eccba to 2a64b8c Compare July 25, 2022 15:35

shonfeder reviewed Aug 2, 2022

View reviewed changes

tatchi force-pushed the auto-id-for-headings branch 3 times, most recently from 8ad77e1 to 5a2d985 Compare August 27, 2022 13:37

tatchi added 10 commits August 29, 2022 11:45

automatically fill the id attributes of the headings

5727396

Fixes: ocaml-community#251

inline Fun.negate impl

a25466b

unicode version of slugify

1a32e7b

add an --auto-identifiers boolean option

2e52f7c

refactor slugify fn and add more tests

a0ce723

trim spaces

0ed7b79

add comment

ad0b2a9

turn off auto_identifiers in tests

36c6f9a

generate unique ids

1fb9404

introduce Identifiers module

ff3b1c8

tatchi force-pushed the auto-id-for-headings branch from 5a2d985 to 621589b Compare August 29, 2022 09:45

switch to another impl

c07e958

tatchi force-pushed the auto-id-for-headings branch from 621589b to c07e958 Compare August 29, 2022 10:05

Use index and accumulator to clean up drop_while

02c6fe8

Thanks to the index and accumulator arguments given by `Uutf.String.fold_utf_8`, we can avoid the need for a sentinel "start" the need for repeated checks after we've found the suffix, and the need to allocate a new string.

shonfeder reviewed Sep 17, 2022

View reviewed changes

Merge branch 'master' into auto-id-for-headings

8f4a3fb

tatchi and others added 4 commits November 3, 2022 22:46

add a test with auto-identifiers option disabled

f5cab78

Merge commit '02c6fe8f07e43f744928fd16d39ac2fc3b3c9b88' into auto-id-…

4cdac96

…for-headings

move some logic into slugify function

820f310

tatchi requested a review from shonfeder November 4, 2022 17:53

shonfeder approved these changes Nov 20, 2022

View reviewed changes

bin/main.ml Outdated Show resolved Hide resolved

Rework auto-identifier doc to be declarative

2096643

shonfeder mentioned this pull request Nov 20, 2022

Automatically fill the id attributes of headings #294

Merged

shonfeder closed this Nov 20, 2022

tatchi mentioned this pull request Nov 24, 2022

move auto generated id into AST #296

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automatically fill the id attributes of the headings #267

automatically fill the id attributes of the headings #267

tatchi commented May 3, 2022 •

edited

Loading

shonfeder left a comment

shonfeder left a comment

shonfeder commented May 13, 2022

tatchi commented May 17, 2022 •

edited

Loading

tatchi commented May 18, 2022

tmattio commented Jul 25, 2022

shonfeder commented Aug 2, 2022 •

edited

Loading

shonfeder left a comment

shonfeder Aug 2, 2022 •

edited

Loading

tatchi Aug 29, 2022

shonfeder left a comment

shonfeder Sep 17, 2022

tatchi Nov 3, 2022

shonfeder left a comment

shonfeder commented Nov 20, 2022

automatically fill the id attributes of the headings #267

automatically fill the id attributes of the headings #267

Conversation

tatchi commented May 3, 2022 • edited Loading

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder commented May 13, 2022

tatchi commented May 17, 2022 • edited Loading

tatchi commented May 18, 2022

tmattio commented Jul 25, 2022

shonfeder commented Aug 2, 2022 • edited Loading

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

tatchi Aug 29, 2022

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder Sep 17, 2022

Choose a reason for hiding this comment

tatchi Nov 3, 2022

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

shonfeder commented Nov 20, 2022

tatchi commented May 3, 2022 •

edited

Loading

tatchi commented May 17, 2022 •

edited

Loading

shonfeder commented Aug 2, 2022 •

edited

Loading

shonfeder Aug 2, 2022 •

edited

Loading