Add a few more tests in anticipation of fixes #1079
Conversation
|
Oh hang on, this needs a fix or two. |
jonludlam
force-pushed
the
more-tests
branch
2 times, most recently
from
75e8f99 to
6007fe0
Compare
jonludlam
added
the
no changelog
|
OK that looks better! |
|
|
||
| let a_show_canonical = | ||
| let doc = "Show modules canonical reference in short output" in | ||
| Arg.(value & flag & info ~doc [ "show-canonical" ]) |
There was a problem hiding this comment.
This options seems unused. Why shouldn't it be true by default ?
|
|
||
| let a_show_removed = | ||
| let doc = "Show removed items in signature expansions in short output." in | ||
| Arg.(value & flag & info ~doc [ "show-removed" ]) |
There was a problem hiding this comment.
This one also seems unused. I think it should be true by default, the output of the tests wouldn't be changed currently.
|
|
||
| let a_long_paths = | ||
| let doc = "Show long paths in short output" in | ||
| Arg.(value & flag & info ~doc [ "long-paths" ]) |
There was a problem hiding this comment.
This option isn't used. What's its use case ?
|
|
||
| let a_show_include_expansions = | ||
| let doc = "Show include expansions in short output" in | ||
| Arg.(value & flag & info ~doc [ "show-include-expansions" ]) |
There was a problem hiding this comment.
What's the inconvenient in merging this option with --show-expansions ? The output is quite short already.
| @@ -0,0 +1,13 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
As I said in #1042, this test is not portable and is annoying to work with due to many files and the scripts. Here's an alternative that should be more portable and is easier to read and change: jonludlam#43
There was a problem hiding this comment.
I find your version slightly less readable, and it's also more tricky to try outside of 'dune runtest'.
There was a problem hiding this comment.
Trying outside of dune runest is not a goal of cram tests. Perhaps the test should exist in two different forms.
Why is it less readable ? It contains basically the same code but with 90% of it removed.
There was a problem hiding this comment.
I prefer my tests to be very explicit and clear, and your version, to me, obfuscates it with the source on one line with shell variable substitution going on. Please can we leave it as it is, and if you have any problems maintaining the test just punt it to me :-D
There was a problem hiding this comment.
Ok, let's leave it as it is.
| if c.short_paths then Format.fprintf ppf "%a" (fn1 c) x | ||
| else Format.fprintf ppf "%s(%a,%a)" txt (fn1 c) x (fn2 c) y | ||
|
|
||
| let wrap2r : |
There was a problem hiding this comment.
I'm a bit worried about the added maintenance in this module. Component changes often and the printing functions are slowing down other work.
These functions need some documentation.
There was a problem hiding this comment.
That's true, I'll add some doc comments.
|
Mostly the extra options were those that were either useful while debugging or I anticipate will be useful in replacing the jq test cases. The marginal cost of adding extra params is dwarfed by the cost of introducing any params at all so I figured I might as well have a few in there now to make the later conversion of tests easier. In addition the defaults are chosen so that 'short' output is shortest! |
|
I think the many options can be surprising while writing tests. What do you think of removing |
|
I kept the 'show-include-expansions' separate from the 'show-expansions' as this is required to show the complete contents of a module, whereas the 'show-expansions' will show everything within the signature, including all submodules. I found this useful particularly when comparing the output with |
- Make it configurable, to allow for shorter output - Improve layout
To ensure we pass on older ocaml versions.
Fix typo Co-authored-by: panglesd <peada@free.fr>
This includes some changes to odoc_print. We might want to change some more tests to use this rather than jq.