Remove infer-timestamp direction #171

Julow · 2019-09-16T15:12:58Z

Implement #169

Instead of setting to-ml or to-md as default, direction is now an option. An exception is raised if this option is needed but not provided.

NathanReb

Looks good!

I just have a minor comment about the error message. Could you also add a test for that new error? It'd be nice, especially if we manage to report the location properly!

NathanReb · 2019-09-18T08:29:55Z

bin/test/main.ml

+    | Some `To_md -> `To_md
+    | Some `To_ml -> `To_ml
+    | None ->
+        Fmt.failwith "--direction must be set to update this file."


I like that it makes --direction optional!

Do you think we could include the location of the block in the markdown file that requires the option to be set? The filename at least would be great. I also think the error message could be slightly more explicit, what would you think about somethng like: "This block must be synchronised with <ml_file>, you must set a promotion direction for <md_file>. See --direction documentation." ?

Improving errors need a bit of work, I'll submit an other PR for easier reviews. For the tests too.
The message can be improved.

Ok, let's leave the location out for now. Could you, as part of this PR, improve the error message a little bit and still add a test just to make sure the error is properly triggered and ensure we don't inadvertently break it in the future?

I believe this can be done quite easily as both file names are available in the scope at this point.

I changed the message.
Location/testing in #172. Not done in this PR because the output would not looks good (uncaught exception) and should be discussed separately.

I'm not sure I understand how that prevents you from adding a test to that PR, especially since the first commit in #172 is pretty much what I'd expect from this PR.

Currently the output is an uncaught exception message from OCaml's runtime, which is not nice. But more importantly, adding this test requires to add a mechanism to run it that may require discussion.
Would you prefer if I reorder the PRs ? #172 first and this one rebased on it ?

NathanReb · 2019-09-18T08:42:21Z

Oh, I'd also like to start keeping the changelog up-to-date from now on. Would you mind adding an entry there with your name and the PR number, under a removed section?

NathanReb · 2019-09-18T09:04:38Z

I'd like to get @samoht approval for this first as well!

CHANGES.md

samoht · 2019-09-18T15:42:46Z

If I understand properly, the option is now mandatory in the presence of file=... metadata? Why not pick a default option instead?

Julow · 2019-09-18T16:05:40Z

That was to avoid choosing the wrong default. Users may not notice this option exists and 50% of them may be surprised of the default since most other options are passed directly as label on each blocks.
Which value should be the default?

NathanReb · 2019-09-18T16:08:08Z

If we must pick a default I'd vote for to-md as I think that's the prefered choice in most cases. It makes more sense to edit ocaml code in .ml and .mli files.

As a matter of fact I'm wondering if there is a good reason for wanting to do it the other way around. Maybe now is the right time to decide whether this option should exist at all.

Julow · 2019-09-18T16:11:24Z

Alternatively, this option could be set on each block, through a label.

NathanReb · 2019-09-18T16:13:57Z

Alternatively, this option could be set on each block, through a label.

I think it make sense to provide that option if we want to allow promotion both ways but that's definitely in the scope for a separate PR.

samoht · 2019-09-18T18:51:42Z

I initially thought that to-ml would make more sense as it allow people to fix typos when reading the .md file only but it's apparently not the preferred one in rwo. In any case we should make the rwo worflow easier by default so to-md as the default seems fine to me.

Julow · 2019-09-19T09:07:37Z

I changed the default to to-md.
I liked that it had no defaults as it's not clear which value make more sense as default and that this option is not working the same way as other options in Mdx (cli instead of per-block).
What do you think about adding a direction label ? Alternatively, to have to-file and from-file instead of the label file to explicitly choose the direction ?

samoht · 2019-09-19T15:13:12Z

I like the to-file and from-file labels

Julow · 2019-09-20T08:28:34Z

I'll implement that.
In the meantime, I think we should merge this. The direction option will have to stay a bit as a transition, so we cannot replace it entirely now.

NathanReb

It looks good now! Thanks!

Let's merge!

@craigfe

CHANGES: #### Added - 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 - Run promotion of markdown files before `.ml` files in generated dune rules (realworldocaml/mdx#140, @clecat) #### Fixed - 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 `infer-timestamp` direction (realworldocaml/mdx#171 @Julow)

@craigfe

CHANGES: #### Added - 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 `infer-timestamp` direction (realworldocaml/mdx#171 @Julow)

@NathanReb

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)

Julow requested a review from NathanReb September 17, 2019 16:16

NathanReb reviewed Sep 18, 2019

View reviewed changes

NathanReb requested a review from samoht September 18, 2019 09:04

Julow commented Sep 18, 2019

View reviewed changes

CHANGES.md Show resolved Hide resolved

Julow mentioned this pull request Sep 18, 2019

Improve error reporting of ocaml-mdx test #172

Merged

Julow force-pushed the remove-infer-timestamp branch from 800490a to 903b1e1 Compare September 18, 2019 13:27

Julow force-pushed the remove-infer-timestamp branch from 903b1e1 to b229602 Compare September 19, 2019 08:53

Julow added 4 commits September 19, 2019 10:54

Remove infer-timestamp direction

e5c4700

Improve message

b4cc0d5

Add CHANGES entry

b229602

Set the default to to-md

3c5204c

NathanReb approved these changes Sep 20, 2019

View reviewed changes

NathanReb merged commit 6efb0dd into realworldocaml:master Sep 20, 2019

This was referenced Sep 20, 2019

Add support for '--syntax' flag in 'ocaml-mdx rule' #177

Merged

Clean up end-to-end testing #178

Merged

NathanReb mentioned this pull request Oct 1, 2019

[new release] mdx (1.5.0) ocaml/opam-repository#14970

Closed

NathanReb mentioned this pull request Oct 3, 2019

[new release] mdx (1.5.0) ocaml/opam-repository#14989

Closed

NathanReb mentioned this pull request Nov 29, 2019

[new release] mdx (1.5.0) ocaml/opam-repository#15415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove infer-timestamp direction #171

Remove infer-timestamp direction #171

Julow commented Sep 16, 2019

NathanReb left a comment

NathanReb Sep 18, 2019

Julow Sep 18, 2019

NathanReb Sep 18, 2019

Julow Sep 18, 2019

NathanReb Sep 18, 2019

Julow Sep 18, 2019

NathanReb commented Sep 18, 2019

NathanReb commented Sep 18, 2019

samoht commented Sep 18, 2019

Julow commented Sep 18, 2019

NathanReb commented Sep 18, 2019

Julow commented Sep 18, 2019

NathanReb commented Sep 18, 2019

samoht commented Sep 18, 2019

Julow commented Sep 19, 2019

samoht commented Sep 19, 2019

Julow commented Sep 20, 2019

NathanReb left a comment

Remove infer-timestamp direction #171

Remove infer-timestamp direction #171

Conversation

Julow commented Sep 16, 2019

NathanReb left a comment

Choose a reason for hiding this comment

NathanReb Sep 18, 2019

Choose a reason for hiding this comment

Julow Sep 18, 2019

Choose a reason for hiding this comment

NathanReb Sep 18, 2019

Choose a reason for hiding this comment

Julow Sep 18, 2019

Choose a reason for hiding this comment

NathanReb Sep 18, 2019

Choose a reason for hiding this comment

Julow Sep 18, 2019

Choose a reason for hiding this comment

NathanReb commented Sep 18, 2019

NathanReb commented Sep 18, 2019

samoht commented Sep 18, 2019

Julow commented Sep 18, 2019

NathanReb commented Sep 18, 2019

Julow commented Sep 18, 2019

NathanReb commented Sep 18, 2019

samoht commented Sep 18, 2019

Julow commented Sep 19, 2019

samoht commented Sep 19, 2019

Julow commented Sep 20, 2019

NathanReb left a comment

Choose a reason for hiding this comment