Initial cleanup and refactoring of the parser #286

shonfeder · 2022-09-03T03:14:50Z

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!

src/parser.ml

The `last` and `tail` functions where each two functions combined into one, differentiated by a `~rev` flag. This imposes unnecessary overhead when trying to read the code (IMO). This change replaces the flag-based usage with two declaritively named functions. We also rename the `ws` function to the more accurate `trim_ws`, since it trims white space, and make the flag a boolean, rather than an optional unit.

Afaik, `drop` is the usual name for this function. Also adds validation logic to ensure the slice cannot exceed the bounds of the underlying string.

Helps reduce cognitive overload in trying to graple with `parser.ml` and gives a more accurate name to the module.

The aim is to enable the reader to see what we are testing for at a glance, to reduce the cognitive load of reading each line.

Replace logic with drop_while and separate into three distinct functions.

- Use uncons to simplify logic - Separate helper function def from case analysis - Simplify failure logic

shonfeder · 2022-09-05T03:47:45Z

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 :)

Allows us to use more recent stdlib methods while maintaining compat with older versions of the compiler.

These will enable cleaner logic in the parsing

Using a fold we can further simplify the logic, and ditch the explicit loop.

shonfeder · 2022-09-06T02:06:24Z

That's what I'll be getting done this weekend! Merging will be blocked until I sort out thierry-martinez/stdcompat#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 · 2022-09-06T02:23:18Z

src/block.ml

@@ -1,5 +1,5 @@
 open Ast
-module Sub = Parser.Sub
+module Sub = StrSlice


I'm not entirely sure what I should do about this naming. Is Sub a good name for the module? I find it cryptic and inaccurate, under the view that it's not really about substrings, per se, but about slices over a base. But maybe that's more of an implementation detail? Really, it's more like an alternative representation of strings based on the view of a slice...

WDYT?

I find StrSlice a better name than Parser.Sub.

Cool. I think I do too. I dropped the Sub alias.

shonfeder · 2022-09-06T02:57:35Z

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.

sonologico · 2022-09-06T22:28:52Z

src/parser.ml

@@ -139,24 +100,23 @@ end = struct

  type 'a t = state -> 'a

+  let ensure_chars_remain st = if st.pos >= String.length st.str then raise Fail


Nitpick, but ensure_chars_remain to me sounds like it prevents chars from being removed whereas it is more like "ensure there are still chars". Can't think of an actual good name. Admittedly not great suggestions:

ensure_remaining_chars

ensure_not_at_end

ensure_end_not_reached

fail_if_no_more_chars

I can see the potential for confusion. Since it's only being used in 2 places, maybe best just to remove this? It also lets us make avoid conditional statements and use conditional expression. WDYT? 6611e25

sonologico · 2022-09-06T22:30:49Z

src/parser.ml

  | Lsetext_heading of int * int
+      (** the level of the heading and how long the underline marker is *)


Should we consider inline records here if they don't make later code more verbose?

One of our compat functions is necessary for backwards compatible functionality.

tatchi · 2022-09-09T15:24:13Z

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 · 2022-09-12T00:19:01Z

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

shonfeder · 2022-09-12T02:37:30Z

dune-project

@@ -24,7 +24,8 @@ Additionally, OMD implements a few Github markdown features, an
 extension mechanism, and some other features. Note that the opam
 package installs both the OMD library and the command line tool `omd`.")
 (tags (org:ocamllabs org:mirage))
- (depends (ocaml (>= 4.05))
+ (depends (ocaml (>= 4.08))


Note that I'm suggesting raising the minimum ocaml version here. I think this library is high-level enough, and has few enough dependencies that we don't need to weigh ourselves down trying to maintain backwards compatibility with versions that much more central libraries have already lost support for.

shonfeder · 2022-09-12T02:38:37Z

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.

tatchi

Nice refactor 👏 Thanks for doing that!

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from f5b759c to aa8fbb0 Compare September 3, 2022 03:43

tatchi reviewed Sep 3, 2022

View reviewed changes

src/parser.ml Outdated Show resolved Hide resolved

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch 2 times, most recently from cfef1b1 to 2dd3255 Compare September 5, 2022 03:45

shonfeder added 16 commits September 4, 2022 23:45

Add some documentation to cryptic Sub functions

e436c57

Rename Sub.tails to Sub.drop

5955a32

Afaik, `drop` is the usual name for this function. Also adds validation logic to ensure the slice cannot exceed the bounds of the underlying string.

Rename "heads" to "take"

147b7d4

Move Sub module into strSlice module file

153bd13

Helps reduce cognitive overload in trying to graple with `parser.ml` and gives a more accurate name to the module.

Add some comments and improve organization

2aaddb3

Factor out repeated length checks

f7bc5b0

Define peek_exn via peek

9b98a62

Document the parser helpers

63a3709

Clean up sp3

ed6f2ae

Remove (most) duplicates of ws testing logic

dabd83c

The aim is to enable the reader to see what we are testing for at a glance, to reduce the cognitive load of reading each line.

Move char preds together

16747cd

Add drop_while and drop_last_while

e842b90

Factor out white space trimming

ec0e52d

Replace logic with drop_while and separate into three distinct functions.

Clarify comment

fa6ae6c

Refactor thematic_break parsing

75805d8

- Use uncons to simplify logic - Separate helper function def from case analysis - Simplify failure logic

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from 2dd3255 to 75805d8 Compare September 5, 2022 03:45

Clean up setext_heading

d3535dc

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from 67204db to 03cb3fb Compare September 5, 2022 17:38

shonfeder added 2 commits September 5, 2022 19:51

Add stdcmpat dependency

60f750b

Allows us to use more recent stdlib methods while maintaining compat with older versions of the compiler.

Remove unneeded disabled warning

28b38e5

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from db0e092 to 28b38e5 Compare September 5, 2022 23:51

shonfeder added 3 commits September 5, 2022 21:48

Add index, split_at, and fold_left

46ee44c

These will enable cleaner logic in the parsing

Further refactor thematic_break

7f88563

Using a fold we can further simplify the logic, and ditch the explicit loop.

Simplify setext_heading again

4a17175

shonfeder requested review from tatchi and sonologico September 6, 2022 02:17

shonfeder self-assigned this Sep 6, 2022

Replace custom Compat module with Stdcompat

4527c6a

shonfeder changed the title ~~WIP: Initial cleanup and refactoring of the parser~~ Initial cleanup and refactoring of the parser Sep 6, 2022

shonfeder marked this pull request as ready for review September 6, 2022 02:20

shonfeder commented Sep 6, 2022

View reviewed changes

Fix formatting

3cafd16

sonologico reviewed Sep 6, 2022

View reviewed changes

shonfeder and others added 6 commits September 7, 2022 21:58

Use inline records for Lsetext_heading

d2666ce

Remove enusre_chars_remain

6611e25

Drop use of "Sub" alias for StrSlice

ff1799d

Add Compat module back

7b1ea60

One of our compat functions is necessary for backwards compatible functionality.

Try to fix name clsah

606e2c0

remove compat module

e1f1f94

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from d7dd919 to b3cb00c Compare September 12, 2022 02:35

shonfeder commented Sep 12, 2022

View reviewed changes

shonfeder requested a review from sonologico September 12, 2022 02:37

tatchi approved these changes Sep 12, 2022

View reviewed changes

Raise minimum ocaml version to 4.08

612f35a

shonfeder force-pushed the shon/parser-refactoring/pass-1 branch from b3cb00c to 612f35a Compare September 12, 2022 13:00

shonfeder enabled auto-merge September 12, 2022 13:08

shonfeder merged commit 7e78fec into master Sep 12, 2022

shonfeder deleted the shon/parser-refactoring/pass-1 branch September 12, 2022 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial cleanup and refactoring of the parser #286

Initial cleanup and refactoring of the parser #286

shonfeder commented Sep 3, 2022 •

edited

Loading

shonfeder commented Sep 5, 2022

shonfeder commented Sep 6, 2022

shonfeder Sep 6, 2022

sonologico Sep 6, 2022

shonfeder Sep 8, 2022

shonfeder commented Sep 6, 2022

sonologico Sep 6, 2022

shonfeder Sep 8, 2022

sonologico Sep 6, 2022

shonfeder Sep 8, 2022

tatchi commented Sep 9, 2022

shonfeder commented Sep 12, 2022

shonfeder Sep 12, 2022

shonfeder commented Sep 12, 2022

tatchi left a comment

		@@ -139,24 +100,23 @@ end = struct

		type 'a t = state -> 'a

		let ensure_chars_remain st = if st.pos >= String.length st.str then raise Fail

		\| Lsetext_heading of int * int
		(** the level of the heading and how long the underline marker is *)

Initial cleanup and refactoring of the parser #286

Initial cleanup and refactoring of the parser #286

Conversation

shonfeder commented Sep 3, 2022 • edited Loading

Context

Reviewing

shonfeder commented Sep 5, 2022

shonfeder commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatchi commented Sep 9, 2022

shonfeder commented Sep 12, 2022

Choose a reason for hiding this comment

shonfeder commented Sep 12, 2022

tatchi left a comment

Choose a reason for hiding this comment

shonfeder commented Sep 3, 2022 •

edited

Loading