doctest allow int/line-number prefix

[S0AndS0]

⚠️ nested case does not spark joy and RegExp might be a better choice for this feature request!

This patch aims to formally begin the discussion here about feature parity between Erlang OTP doctest, and the library of the same name maintained by williamthome

Specifically this patch targets > vs 1> syntax for declaring the start of a testable code line or set of lines, ex;

Erlang OTP doctest

-doc """
This test is how we do with `erlang/otp` doctests

```
> true.
true
> totallyOkay.
totallyOkay
```
"""
woot() -> ok.

williamthome doctest

-doc """
This test is how we do with `williamthome/doctest` (prior to version `0.10.0`)

```erlang
1> false.
false
2> thisIsFine.
thisIsFine
```
"""
muchJoy() -> ok.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

CT Test Results

    2 files     97 suites   1h 8m 53s ⏱️
2 195 tests 2 146 ✅ 47 💤 2 ❌
2 562 runs  2 511 ✅ 49 💤 2 ❌

For more details on these failures, see this check.

Results for commit 9ed5b46.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[S0AndS0]

Here are some results of manual testing, because I'm not yet smort enough to run only doctests from the CLI;

• Mixing of > and INT> seem fine, so I've not broken anything... yet

  > parse_tests([<<"> SomeCode.">>, <<"419> OtherCode.">>], []).
  [{test,[<<"SomeCode.">>,10,<<"OtherCode.">>],"_"}]

• Multi-line code seems to work fine too

  > parse_tests([<<"68> ReallyLongLine,">>, <<" ConinuedOnAnotherLine.">>], []).
  [{test,[<<"ReallyLongLine,">>,10,
          <<"ConinuedOnAnotherLine.">>],
         "_"}]
  > parse_tests([<<"> ReallyLongLine,">>, <<" ConinuedOnAnotherLine.">>], []).
  [{test,[<<"ReallyLongLine,">>,10,
          <<"ConinuedOnAnotherLine.">>],
         "_"}]

• Lines that contain > but not prefixed by an integer are ignored as usual

  > parse_tests([<<"Something > OtherThang.">>], []).
  [{test,[],[<<"Something > OtherThang.">>]}]

• 🐛 Lines prefixed by any number of blank space are considered code

  > parse_tests([<<" wat.">>], []).
  [{test,[<<"wat.">>],"_"}]

  Note; double-checking against pre-PR code this is true there too 🤷

I still be learning the ways of Erlang, and least-bad practices, so any tips towards reducing future costs to maintenance are very much appreciated!

CC: @garazdawi I figured it'd be wise to nudge things specific to OTP parity from williamthome/doctest -- Issue 47 to here ;-)

[garazdawi]

One of the things I like a lot about doctests is that it adds uniformity to the examples. So I think that I would like it to be very oppionionated about how you write examples. So I would like for the examples to either use > or 1>, not a mix. The 1> adds one extra unnecessary number in front, but it makes the examples closer to what you normally see in an Erlang shell.

Which one do you prefer and why?

[garazdawi]

Here are some results of manual testing, because I'm not yet smort enough to run only doctests from the CLI;

1> shell_docs:test(ModuleToTest, []).

[S0AndS0]

Which one do you prefer and why?

For compatibility with preexisting doctest tooling and tests I prefer number prefixed code lines, also it should hopefully mean fewer examples need edited. Plus having an incrementing number makes it obvious to readers, and copy/pasters, of examples that there likely is an intended order. Ex;

1> State = "World".
2> Greeter = fun(State) -> "Hello " ++ State end.
3> Result = Greeter(State).
"Hello World"

At the same time I prefer >, without a line number prefix, because it could be used in doc-comments/tests where asserts are independent of one another, ex;

> true.
true
> false.
false

True having both is a more overhead to maintain and parse, and I'm but one opinionated noob ;-)

[williamthome]

I think just the > is fine and less tedious. Elixir uses the iex> prefix and optionally allows the line numbers. See examples.

Maybe it would be nice to use erl> as the prefix to distinguish between Erlang and Elixir examples when they are on the same documentation, but I'm for the simplicity of the > prefix.

---

@garazdawi Sorry for the off-topic question below:

Is it possible for the shell to capture pasted content (Ctrl + v)? If so, it would be great if the shell could recognize the first character, and if it is a >, paste all the lines starting with the > in sequence. For instance:

```erlang
> A = 1.
1
> B = 1.
> A + B.
2
```

The shell would then print:

1. A = 1.
2. Enter
3. B = 1.
4. Enter
5. A + B.
6. Enter

It would be nice to have this feature to experiment with copied examples from the documentation.

[michalmuskala]

For the second point, the way it works with ex_doc, is that when you copy the example, the iex> parts are not actually copied. I'm sure something similar could work for Erlang examples

[S0AndS0]

Ooof, so seems to be lots of variants that could be supported, but could don't mean should... 🤔 still I'm game to be nerd-sniped for a bit ;-)

TLDR RE = "^(\\w*(?:\\(\\d+\\))?)(>)" and re:split seem to cover all conditions! We either get a list of length 4 with last element being the code, or list of length 1 which should be a different branch's responsibility.

Here's a synopsis of what I could find of variants;

• Elixer ExUnit v1.12.3
  • iex> starter with ...> code continuation
  • iex(1)> starter with ...(1)> code continuation
• Erlang williamthome/doctest:v0.9.3
  • 1> starter with .. code continuation
• Erlang williamthome/doctest:v0.10.2
  • 1> starter with .. code continuation
  • > starter with .. code continuation
• Erlang OTP 29138f5dd1:lib/stdlib/src/shell_docs_test.erl
  • > starter with code continuation

... Double-checking dates it seems williamthome/doctest is about nine-months old, so perhaps my thinking about backwards compatibility and smooth transition should be adjusted, because the code continuation differences would need tackled too.

Simplicity is nice, and usually I'm all for it, but I like the idea of broader support for documentation tests too 🤷

[williamthome]

Just checked and the iex> prefix in Elixir is because of the Elixir shell (v1.17):

iex(1)> a = ~s"
...(1)> foo
...(1)> "
"\nfoo\n"
iex(2)> b =
...(2)> 1 +
...(2)> 1
2
iex(3)> c = :foo
:foo

The line numbers can be omitted in Elixir doctest:

iex> a = ~s"
...> foo
...> "
"\nfoo\n"
iex> b =
...> 1 +
...> 1
2
iex> c = :foo
:foo

---

In Erlang shell (OTP-28) it is:

1> A = ~"
   foo
   ".
<<"\nfoo\n">>
2> B =
   1 +
   1.
2
3> C = foo.
foo

Omitting line numbers in Erlang doctest:

> A = ~"
  foo
  ".
<<"\nfoo\n">>
> B =
  1 +
  1.
2
> C = foo.
foo

Erlang should not always adhere to what's implemented in Elixir, but maybe the optional line count support is a good idea. I do not have a strong opinion of the "why", only because of the shell ;)

[S0AndS0]

I do not have a strong opinion of the "why", only because of the shell ;)

Having noodled it about in my brain, and skimmed bits of doc-comments within various Erlang OTP sources, I believe the strongest argument I can think of for supporting line-numbers in doctests is reducing Git Diff churn.

Here be but two examples I could find that should just work were we to support both ways;

• erlang/otp/lib/stdlib/src/re.erl

  1> re:run("aaaaaaaaaaaaaz","(a+)*z").
  {match,[{0,14},{0,13}]}
  2> re:run("aaaaaaaaaaaaaz","(a+)*z",[{match_limit_recursion,5}]).
  nomatch
  3> re:run("aaaaaaaaaaaaaz","(a+)*z",[{match_limit_recursion,5},report_errors]).
  {error,match_limit_recursion}

• erlang/otp/lib/stdlib/src/binary.erl

  > binary:bin_to_list(<<"erlang">>, {1,3}).
  "rla"
  %% or [114,108,97] in list notation.

And though glancing elsewhere I found a few instances where either would require some touch-work, ex;

• erlang/otp/lib/stdlib/src/argparse.erl

  1> argparse:parse([], #{arguments => [#{name => myarg, short => $m}]}).
  
  {ok,#{}, ...
  2> argparse:parse([], #{arguments => [#{name => myarg, short => $m, default => "def"}]}).
  
  {ok,#{myarg => "def"}, ...

... I've got a hunch it'll be less touch-work than picking but one style.

[garazdawi]

I do not have a strong opinion of the "why", only because of the shell ;)

Lets do what the shell does, so that we get a similar look and feel as the shell. That is the regexp to match a test start is [0-9]+> . It will cause a bit of churn to change all the modules already using the new doctests, but better now than later. Will you do that change @S0AndS0 ?

For the second point, the way it works with ex_doc, is that when you copy the example, the iex> parts are not actually copied. I'm sure something similar could work for Erlang examples.

@michalmuskala it already works like this :)

[S0AndS0]

@garazdawi
Lets do what the shell does... That is the regexp to match a test start is [0-9]+> ... Will you do that change @S0AndS0?

Sure thing! I'll try to get something prettier (more maintainable than my current questionable code suggestions) sorted and submitted for public comment soon™

[S0AndS0]

TLDR update I'm blocked by something upstream of shell_docs_test:run_test/2 filtering-out doctest blocks that include line numbers, and though RegExp mostly works as intended something within run_tests(Test, Bindings) or downstream of it does not like the format of {test, ...} tuples edits have returned.

A hint, or two, would be much appreciated!

Unabridged update

I think I won the battle with RegExp, and by doing so also flattened that pesky case statement out of existence!

Like the most recent commit message says, local/isolated testing of shell_docs_test:parse_tests/2 shows changes to be behaving themselves. However, I'm still a bit touched by "skill issues" for getting inputs injected into the changes correctly. Near as I can tell, after adding some io:format/2 sanity checks, the shell_docs_test:test/2 and/or shell_docs_test:run_test/2 functions are now needing attention 🤔

If we get doctest input similar to;

-module(wat).

-export([woot/0]).

-doc """
```erlang
> wat:woot().
false
```
""".
woot() -> woot.

... Then I can observe the run_test(Code, InitialBindings) function gets the doctest lines, but requested RegExp changes means > wat:woot() is filtered out. And adding a line number shows shell_docs_test:run_test/2 never gets any doctest lines, ex;

 -doc """
 ```erlang
-> wat:woot().
+1> wat:woot().
 false
 ```
 """.
 woot() -> woot.

Altering the RegExp to allow for optional line-numbers, (?'line_number'[0-9]+) → (?'line_number'(?:[0-9]+)?), and not using line-numbers in doctests, ex...

--define(RE_CAPTURE, "(?'indent'^\s*)(((?'line_number'[0-9]+)(?'prefix'>\s|\s))|(?'prefix'%))(?'content'[^%]*)(?'comment'(?:.*%.*)?)").
+-define(RE_CAPTURE, "(?'indent'^\s*)(((?'line_number'(?:[0-9]+)?)(?'prefix'>\s|\s))|(?'prefix'%))(?'content'[^%]*)(?'comment'(?:.*%.*)?)").
 -define(RE_OPTIONS, [ {capture, [indent, line_number, prefix, content, comment] ,binary}, dupnames ]).
 parse_tests([], []) ->

... does show things are mostly parsed as they use-to. I.E. [{test, ...}] filters back up to shell_docs_test:run_test/2 but then I'm not observing expected failure of woot =:= false comparison.

---

Edit 🤦 test({pre,[],[{code,Attrs,[<<">",_/binary>> = Code]}]}, Bindings) is what be pre-filtering

[garazdawi]

Let me know when you are ready for a review!

The examples in lists, binary, maps, zstd and shell_docs_test also need to be updated to follow the new style.

[S0AndS0]

Thanks for being patient with me!

I think I've got most of the {match, ...} things sorted, and will be happy to update doctests that were previously functional too.

For single line test/match blocks it seems to be mostly doing what it should... mostly... but for reasons I've yet to figure-out it ain't at all happy with mutli-line stuff and I either may need a bit of help or more time to resolve my skill issues x-)

[S0AndS0]

I think (hope) changes are ready for review!

Turns out I was over complicating things with the RegExp and stuff... no big surprise there (-x

Updated doctest comments to include line numbers and adjusted indentation too for

• lib/stdlib/src/binary.erl
• lib/stdlib/src/lists.erl
• lib/stdlib/src/maps.erl
• [¿?] lib/stdlib/src/shell_docs_test.erl please check 49765b5d1e commit message
• lib/stdlib/src/zstd.erl

Please do let me know if I've broken anything, that wasn't already misbehaving, and I'll do my best to make corrections!

[garazdawi]

We did some changes in the lists module, would you mind rebasing on latest master?

I’ll have a closer look on Monday.

[S0AndS0]

I don't mind at all, and ya can consider the rebasing done :-)

Looking forward to Monday's review I will, and 'ntil then I'll be wishing y'all a wonderful weekend!