search

ocaml / omd

extensible Markdown library and tool in "pure OCaml"
ISC License
156 stars 45 forks source link

Initial cleanup and refactoring of the parser #286

Closed shonfeder closed 1 year ago

shonfeder commented 2 years ago

Context

To help clear the way for #223 we've agreed that some substantial refactoring of the parer passes should be undertaken to make it less unwieldy to extend and reason about (following the principle of "first make the change easy, then make the change").

This work is largely just preparatory, and the result of "active reading", which is adding comments, cleanup, and simplification as I study the existing parser. I'll resume this work later in the week.

Reviewing

There was some churn in the changes, but review still may benefit from skimming each commit, before a final review, to see the evolution of the changes.

I'd particularly like the review to ensure that the changes I'm proposing actually make the code more readable and easier to reason about. I don't want to be introducing changes for their own sake, so if you think any of my changes end up just as complicated or hard to reason about as what they are replacing (including the naming suggestions), please raise alarm!

Thanks in advance for the review!

shonfeder commented 2 years ago

It's a long weekend in Canada, so I'll keep doing the cleanup here thru tomorrow, then I'll offer up whatever I have done by EOD for reviw :)

shonfeder commented 2 years ago

That's what I'll be getting done this weekend! Merging will be blocked until I sort out https://github.com/thierry-martinez/stdcompat/issues/26, but this should be ready for review (mainly just to keep it from getting too long).

I'll resume this superficial pass of parsing later in the week, or on the weekend at the latest. I think I'm forming some ideas for more substantial refactoring, and I'll open issues to plan those out as they become more formed.

shonfeder commented 2 years ago

Interesting! It looks like some of our custom Compat functions are compatible with 4.14, but break behavior related to unicode on older versions of OCaml! I'll have to restore our Compat, probably as an overlay on top of Stdcompat, and just for whichever function is doing the magic here.

tatchi commented 1 year ago

Interesting! It looks like some of our custom Compat functions are compatible with 4.14, but break behavior related to unicode on older versions of OCaml! I'll have to restore our Compat, probably as an overlay on top of Stdcompat, and just for whichever function is doing the magic here.

Another option could be to use the Uutf.Buffer.add_utf_8 function instead (and get rid of the Compat module). See my commit

shonfeder commented 1 year ago

That's perfect. Thanks for the fix, @tatchi!

shonfeder commented 1 year ago

Thanks to @tatchi's suggestion and my suggestion to increase our minimum OCaml version to 4.08 (for the time being at least) I think this should be passing all CI now and ready for another review.