Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a --output/-o option to specify mdx test output #194

Merged
merged 2 commits into from
Nov 21, 2019

Conversation

NathanReb
Copy link
Contributor

Fixes #193

This PR adds a --output/-o option to set the output for the ocaml-mdx test command.

It used to always write to <input_file>.corrected so the default behaviour is preserved, i.e. if you do not pass the --output option, it still writes to that file.
If passed --output -, the command will write the corrected file to stdout instead of writing it to a file. Setting stdout as the output implies --force-output. This seems like the best behaviour if you want to pipe it into some other command or redirect it to a file for later diffing.

I deleted the code of Misc.run_expect_test (which isn't part of the pulbic API) since it was only used in Mdx.run. Instead I created a more generic function Mdx.run_str which returns a pair composed of the test result (whether the corrected version is equal to the input or not) and the corrected content.

It is used by two new functions Mdx.run_to_stdout and Mdx.run_to_file. The former just writes the corrected content to stdout no matter what, the second preserves the behaviour of Misc.run except that it takes an outfile argument.

I wasn't too sure about what needed to be done with the "clean up" behaviour which takes care of deleting the corrected file if it exists and the test succeeded so I kept it for any output file, whether it's the default are an explicitly specified one. I don't think it's necessary to do that when used through dune as it doesn't use the file presence alone to determine which files need to be promoted but that's unrelated to this PR.

Finally, to preserve the public API, I exposed these two new functions and re-implemented Mdx.run on top of Mdx.run_to_file so that its behaviour and signature remain the same.

I also added tests to ensure that this option works properly.

Let me know what you think!

@NathanReb NathanReb requested review from samoht and Julow November 15, 2019 14:11
@NathanReb NathanReb force-pushed the mdx-test-output-option branch from abd9f6f to e18ea90 Compare November 15, 2019 14:12
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should --output implies --force-output in every cases ?
If I specify the output file on the CLI, I don't expect it to be not create or even removed.

bin/test/main.ml Outdated Show resolved Hide resolved
bin/test/main.ml Outdated Show resolved Hide resolved
bin/test/main.ml Outdated Show resolved Hide resolved
lib/mdx.mli Show resolved Hide resolved
@samoht
Copy link
Collaborator

samoht commented Nov 18, 2019

Can you use the compat layer to keep support for 4.02? Thanks

# File "bin/cli.ml", line 113, characters 17-19:
# Error: Unbound constructor Ok

@NathanReb
Copy link
Contributor Author

I think force-output should always be true, that would spare some effort on our hand and offer a more consistent behaviour but let's keep that for a separate PR!

@NathanReb NathanReb force-pushed the mdx-test-output-option branch from d1b4533 to 81c2813 Compare November 19, 2019 10:03
@NathanReb
Copy link
Contributor Author

Can you use the compat layer to keep support for 4.02? Thanks

Done!

@Julow
Copy link
Collaborator

Julow commented Nov 19, 2019

I think force-output should always be true, that would spare some effort on our hand and offer a more consistent behaviour but let's keep that for a separate PR!

I meant setting force-output when --output is set. Changing the default behavior requires an other PR but this is the behavior of --output that I'm talking about. Let's not implement legacy code.

@NathanReb
Copy link
Contributor Author

I understood what you meant, my point was that we can just tackle one thing at a time. Having --force-output implied when output is specified means having completely different behaviours for --output and the default which currently is just --output <input>.corrected.

I'd rather drop force-output completely in a future release. Let's keep the breaking changes to a minimum for the next one.

@NathanReb NathanReb force-pushed the mdx-test-output-option branch from 81c2813 to f385e6c Compare November 19, 2019 10:41
@Julow
Copy link
Collaborator

Julow commented Nov 19, 2019

I don't agree. The default feels like --output <input>.corrected but it doesn't have to be !
My point was that this has nothing to do with the default behavior and it should be discussed and implemented correctly the first time.
This behavior (not writing a file, or worse, removing it when, as a user, I wrote it on the command-line) is not right for me.
You already implemented a special case for --output -.

@NathanReb
Copy link
Contributor Author

Yeah it's a good point, I just fixed it!

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good !

@NathanReb NathanReb merged commit 7e912f1 into realworldocaml:master Nov 21, 2019
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Nov 29, 2019
CHANGES:

#### Added

- Add a `--output`/`-o` option to the `test` subcommand to allow specifying a different
  output file to write the corrected to, or to write it to the standard output (realworldocaml/mdx#194, @NathanReb)
- Migrate to OCaml 4.08 AST to add support for `let*` bindings (realworldocaml/mdx#190, @gpetiot)
- Add `--syntax` option to `rule` subcommand to allow generating rules for cram
  tests (realworldocaml/mdx#177, @craigfe)
- Add a `require-package` label to explicitly declare dune `package` dependencies of a code block
  (realworldocaml/mdx#149, @Julow)
- Add an `unset-` label to unset env variables in shell blocks (realworldocaml/mdx#132, @clecat)

#### Changed

- Format rules generated by `ocaml-mdx rule` using `dune format-dune-file` (realworldocaml/mdx#184, @NathanReb)
- Run promotion of markdown files before `.ml` files in generated dune rules (realworldocaml/mdx#140, @clecat)

#### Fixed

- Use module_presence information on Env.summary to prevent fetching absent modules from the
  toplevel env (realworldocaml/mdx#186, @clecat)
- Remove trailing whitespaces at the end of toplevel or bash evaluation lines
  (realworldocaml/mdx#166, @clecat)
- Improve error reporting of ocaml-mdx test (realworldocaml/mdx#172, @Julow)
- Rule: Pass the --section option to `test` (realworldocaml/mdx#176, @Julow)
- Remove trailing whitespaces from shell outputs and toplevel evals (realworldocaml/mdx#166, @clecat)
- Remove inappropriate empty lines in generated dune rules (realworldocaml/mdx#163, @Julow)
- Fix ignored `skip` label in `ocaml-mdx pp` (realworldocaml/mdx#1561, @craigfe)
- Fix synchronization of new parts from markdown to `.ml` (realworldocaml/mdx#156, @Julow)
- Fix ignored `[@@@Parts ...]` markers within module definitions (realworldocaml/mdx#155, @Julow)
- Fix a bug in internal OCaml version comparison that lead to crashes in some cases (realworldocaml/mdx#145, @gpetiot)
- Promote to empty `.ml` file when using `to-ml` direction (realworldocaml/mdx#139, @clecat)
- Apply `--force-output` to `.ml` file as well (realworldocaml/mdx#137, @clecat)
- Fix a bug preventing `.corrected` files to be written in some cases (realworldocaml/mdx#136, @clecat)
- Add compatibility with `4.09.0` (realworldocaml/mdx#133, @xclerc)

#### Removed

- Remove the `output` subcommand as it was very specific to RealWorldOCaml needs (realworldocaml/mdx#195, @NathanReb)
- Remove the `infer-timestamp` direction (realworldocaml/mdx#171 @Julow)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow more configuration of the test command
3 participants