Improve user error messages #1147

Julow · 2019-11-14T11:40:36Z

Add tests for invalid ocamlformat files and env variable.
Nicer error messages instead of uncaught exceptions with sexp payload and backtrace
Error messages are printed to stderr

facebook-github-bot · 2019-11-14T11:40:47Z

Hi Julow! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2019-11-14T11:52:44Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

emillon

changes look good, thanks for working on this

lib/Conf.ml

emillon · 2019-11-14T13:24:13Z

lib/Reason.ml

@@ -52,10 +52,10 @@ module Binary_reason = struct
    let rec loop = function
      | [] ->
          if is_intf_or_impl magic then
-            user_error "Unknown version" [("magic", Sexp.Atom magic)]
+            failwith (Format.sprintf "Unknown version (magic: %s)" magic)


what do you think about defining failwithf in Import? (using Format.kasprintf failwith). I thought it was in Base, but it's only in Core.

This would only be used twice. In the future, this code could be either gone or improved, in both cases this would no longer be useful.

I'm fine with a dead failwithf in import if you think it's worth it in the meantime.

Yes it would not be in vain, in the future we would need an easy way to emit errors, warnings and hints

Yes, that's why I'm against adding this.
We need more than the Failure exception (eg. locations, colors, formatting) and I don't want that to be used elsewhere.

I see no problem to have only what is necessary for this PR for now and rework it when we need it

emillon · 2019-11-14T13:25:41Z

test/cli/test_cli.expected

+================
+args: --impl -
+ocamlformat: Error while parsing .ocamlformat:
+             For option "bad": expecting "0.12-18-gea9fdbb-dirty" but got "bad"


This will always fail as it depends on the commit number, unfortunately. You can postprocess the output but we can leave this test for now I think.

I was hoping #1133 would solve this.
I just forgot to mark this PR as "Draft".

emillon · 2019-11-14T13:34:52Z

test/cli/test_cli.ml

+      | Some d -> Format.fprintf pp "cd '%s'; " d
+      | None -> ()
+    in
+    let cmd = Format.asprintf "%a%s %s 2>&1" pp_cd pwd ocamlformat args in


This adds a lot of complexity to the test harness just to parse .ocamlformat files. We could use spawn to simplify that, but actually, does this need to be tested at the end to end level?

What do you think about pulling out the function that parses a .ocamlformat file and put that in a test harness instead? It looks like it's the main branch of Conf.read_config_file. That way, we can just test a string -> config file and remove all of this.

Yes. That's something I want to do next and it's required to have ocamlformat as a library.

Here I wanted to test end-to-end because I'm checking the error message given to the user.
I think we should both keep this and have the config parsing unit-tested.

It's not too useful to test the same thing at both level. In my experience it's enough to just test the happy path as e2e (and maybe one error to see how it's rendered), but that has not its place in this test harness. We can think how to make an e2e test for file parsing (ocamlformat / ignore / ocp-indent) in a separate PR. (we can probably do it by hand)

(you'll need #1148 to link a test suite, I can give you a hand with that)

Yes but here I'm not testing that errors are reported or anything. I'm testing the rendering of messages.
I'm testing a single level, the e2e one.
Unit-tests are for an other PR, this one is about error messages.

If dune can do more work for us that would be great, I'm not opposed to having duplicates of tests in many places for now. That's a job we need to do later anyway, determine where and how is tested every usecase, and fill the blanks.

yes, we can live with extra tests. however I don't think we should try to use this test harness for things it'll do worse than dune. to expand a bit:

in Add command line tests #1117 we added a way to run ocamlformat with some options and stdin and compare the results.

in Use an expect test for cli tests #1126 this was simplified since running a binary with arguments is simple enough that it can be done in-process. expect test are a good enough balance for this.

However, if we need to add features like changing directory, setting environment variables, etc, this is more complicated in-process than using (setenv) and (chdir) in a dune file.

So I suggest that we:

leave test_cli unchanged in this PR and use it only for things that just pass arguments and don't need to set env/read .ocamlformat files

use manually written dune rules for these new tests, in a style similar to the rules generated in Add command line tests #1117. we don't need a generator for this.

As for test_cli, maybe it was a bit overreaching, and the right balance is a set of manually written dune rules (without a generator). There are not that many cases, so we can avoid having this test_cli and leverage dune's ability to run processes with arguments and diff files. I'm happy to undo this part after this PR if that works for you.

I agree test_cli is becoming too complicated (it was from the beginning) and we will have to maintain it.
Instead of going back to vanilla dune (it still lacks features like allowing failing commands) and is a bit harder to setup when we have new needs (especially with no generators), wouldn't it be better to just use better tools ?
We could use bos to cleanup process-handling code or use a cram-like tool (eg. ppx_expect if it can do that) ?

OK, great. We can live with a couple manual rules. I suggest the following plan to move things forward:

you convert the new tests to plain dune rules (in this PR)

I undo test_cli as plain dune rules (in another PR)

we try to use craml or mdx later to run the commands (later)

sounds good?

I'm not sure converting tests to plain dune rules is good idea because it's pretty hard and time consuming.
This PR won't be merged now because of something else. What about:

Keep this PR open

We try to use craml or mdx (in another PR) for the tests currently in master

Rebase this PR on that
?

test/cli/test_cli.ml

gpetiot

Looks good to me if the test duplication is not blocking, can be merged as soon as it is ready.

Julow · 2019-11-15T16:07:55Z

As I was arguing, I believe there is no problem with the tests. This new tests are about how error are printed to CLI users and are absolutely not testing the handling of errors.

This cannot be merged yet, the version test is not stable. I'm hoping #1133 will help with that, I think the version check should be relaxed with dev builds.

jberdine · 2019-11-24T21:00:05Z

This cannot be merged yet, the version test is not stable.

Is the (only) problem that there is a test which includes the current version in the output error message, and that keeps changing? If so, is it possible to change that test to just check the exit code?

Julow · 2019-11-25T10:26:50Z

Is the (only) problem that there is a test which includes the current version in the output error message, and that keeps changing? If so, is it possible to change that test to just check the exit code?

Yes, #1133 needs more work so I'll workaround that !

Julow · 2019-11-25T12:55:16Z

I rebased on master (cli tests changed) and worked around the version problem.
This is ready for review and merge.

test/cli/dune

jberdine · 2019-11-25T15:37:19Z

I haven't managed to find the root cause, but note that with this PR I still see the emacs integration silently swallowing errors.

lib/Conf.ml

jberdine · 2019-11-25T17:09:22Z

Hmm, I don't understand why, but the disagreement between the changes from 1127 and ocamlformat.el still remain after this.

Julow · 2019-11-26T10:45:31Z

@jberdine What is the error that you expect and don't show up? Do you see the Could not apply ocamlformat message?

jberdine · 2019-11-26T14:32:23Z

When I test with a file whose config specifies a mismatched version, for example, and invoke ocamlformat, I see "Applied ocamlformat" when in fact it errored out. I tried debugging the elisp for a while, and saw that printing the status code returned from call-process is 0. So I don't understand at all.

Julow · 2019-11-26T17:23:37Z

Could it be that the file passed to ocamlformat is not located where the bad .ocamlformat file can be found? (eg. in /tmp)

jberdine · 2019-11-26T17:44:34Z

I don't think that is it. I have tested by building the parent commit of pr1127 and see the expected error, and then build pr1127 and the error is silently ignored, and with this pr and see the same as with 1127. I just pick any random infer file, add some extra line, try to reformat and look for the error in the echo area or *compilation* buffer depending on the emacs config (I've tried both).

Julow · 2019-12-09T14:29:00Z

This PR wasn't fixing #1139. I opened #1180 for that.

I think this PR is still an improvement, it improves error outputs a lot and adds tests for them.

gpetiot · 2019-12-12T12:46:29Z

If all blocking points have been resolved it's time to merge this one as well.

Print error messages to stderr Remove sexp output and backtraces Add tests for some error messages

Julow · 2019-12-12T13:30:12Z

Just rebased. I think we can merge now :)

CHANGES: #### New features + Add an option `--margin-check` to emit a warning if the formatted output exceeds the margin (ocaml-ppx/ocamlformat#1110) (Guillaume Petiot) + Preserve comment indentation when `wrap-comments` is unset (ocaml-ppx/ocamlformat#1138, ocaml-ppx/ocamlformat#1159) (Jules Aguillon) + Improve error messages (ocaml-ppx/ocamlformat#1147) (Jules Aguillon) + Display standard output in the emacs plugin even when ocamlformat does not fail (ocaml-ppx/ocamlformat#1189) (Guillaume Petiot) #### Removed + Remove `ocamlformat_reason` (ocaml-ppx/ocamlformat#254, ocaml-ppx/ocamlformat#1185) (Etienne Millon). This tool has never been released to opam, has no known users, and overlaps with what `refmt` can do. + Remove `ocamlformat-diff` (ocaml-ppx/ocamlformat#1205) (Guillaume Petiot) This tool has never been released to opam, has no known users, and overlaps with what `merge-fmt` can do. #### Packaging + Work with base v0.13.0 (ocaml-ppx/ocamlformat#1163) (Jules Aguillon) #### Bug fixes + Fix placement of comments just before a '|' (ocaml-ppx/ocamlformat#1203) (Jules Aguillon) + Fix build version detection when building in the absence of a git root (ocaml-ppx/ocamlformat#1198) (Anil Madhavapeddy) + Fix wrapping of or-patterns in presence of comments with `break-cases=fit` (ocaml-ppx/ocamlformat#1167) (Jules Aguillon) This also fixes an unstable comment bug in or-patterns + Fix an unstable comment bug in variant declarations (ocaml-ppx/ocamlformat#1108) (Jules Aguillon) + Fix: break multiline comments (ocaml-ppx/ocamlformat#1122) (Guillaume Petiot) + Fix: types on named arguments were wrapped incorrectly when preceding comments (ocaml-ppx/ocamlformat#1124) (Guillaume Petiot) + Fix the indentation produced by max-indent (ocaml-ppx/ocamlformat#1118) (Guillaume Petiot) + Fix break after Psig_include depending on presence of docstring (ocaml-ppx/ocamlformat#1125) (Guillaume Petiot) + Remove some calls to if_newline and break_unless_newline and fix break before closing brackets (ocaml-ppx/ocamlformat#1168) (Guillaume Petiot) + Fix unstable cmt in or-pattern (ocaml-ppx/ocamlformat#1173) (Guillaume Petiot) + Fix location of comment attached to the underscore of an open record (ocaml-ppx/ocamlformat#1208) (Guillaume Petiot) + Fix parentheses around optional module parameter (ocaml-ppx/ocamlformat#1212) (Christian Barcenas) + Fix grouping of horizontally aligned comments (ocaml-ppx/ocamlformat#1209) (Guillaume Petiot) + Fix dropped comments around module pack expressions (ocaml-ppx/ocamlformat#1214) (Jules Aguillon) + Fix regression of comment position in list patterns (ocaml-ppx/ocamlformat#1141) (Josh Berdine) + Fix: adjust definition of Location.is_single_line to reflect margin (ocaml-ppx/ocamlformat#1102) (Josh Berdine) #### Documentation + Fix documentation of option `version-check` (ocaml-ppx/ocamlformat#1135) (Wilfred Hughes) + Fix hint when using `break-separators=after-and-docked` (ocaml-ppx/ocamlformat#1130) (Greta Yorsh)

Print error messages to stderr Remove sexp output and backtraces Add tests for some error messages

Julow requested review from emillon and gpetiot November 14, 2019 11:40

facebook-github-bot added the CLA Signed label Nov 14, 2019

emillon reviewed Nov 14, 2019

View reviewed changes

gpetiot approved these changes Nov 15, 2019

View reviewed changes

Julow mentioned this pull request Nov 15, 2019

Use dune-build-info to compute version number #1133

Merged

emillon mentioned this pull request Nov 19, 2019

CLI tests: use dune to run ocamlformat #1157

Merged

Julow force-pushed the refactor_errors branch from 6bdc881 to 732fb6a Compare November 25, 2019 12:50

Julow requested a review from gpetiot November 25, 2019 12:54

jberdine reviewed Nov 25, 2019

View reviewed changes

test/cli/dune Outdated Show resolved Hide resolved

jberdine reviewed Nov 25, 2019

View reviewed changes

lib/Conf.ml Outdated Show resolved Hide resolved

Julow force-pushed the refactor_errors branch from a041e37 to 08a5c59 Compare December 9, 2019 14:26

Julow force-pushed the refactor_errors branch from 08a5c59 to 2917c91 Compare December 12, 2019 13:21

Julow added 3 commits December 12, 2019 14:21

Improve user_error messages

8547ffa

Print error messages to stderr Remove sexp output and backtraces Add tests for some error messages

Neutralize bad_version test

9e4400a

Typo in version msg

946518e

Julow added 4 commits December 12, 2019 14:23

Update CHANGES

f4237a2

Adjust comment

0a00993

fmt

2917c91

Rebase fail

77260f9

Julow merged commit 39897bd into ocaml-ppx:master Dec 12, 2019

Julow mentioned this pull request Jan 28, 2020

[new release] ocamlformat (0.13.0) ocaml/opam-repository#15753

Merged

bogdan2412 pushed a commit to bogdan2412/ocamlformat that referenced this pull request Mar 28, 2020

Improve user error messages (ocaml-ppx#1147)

fb5f8f7

Print error messages to stderr Remove sexp output and backtraces Add tests for some error messages

Improve user error messages #1147

Improve user error messages #1147

Conversation

Julow commented Nov 14, 2019 • edited Loading

facebook-github-bot commented Nov 14, 2019

facebook-github-bot commented Nov 14, 2019

emillon left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpetiot left a comment

Choose a reason for hiding this comment

Julow commented Nov 15, 2019

jberdine commented Nov 24, 2019

Julow commented Nov 25, 2019

Julow commented Nov 25, 2019

jberdine commented Nov 25, 2019

jberdine commented Nov 25, 2019

Julow commented Nov 26, 2019

jberdine commented Nov 26, 2019

Julow commented Nov 26, 2019

jberdine commented Nov 26, 2019

Julow commented Dec 9, 2019

gpetiot commented Dec 12, 2019

Julow commented Dec 12, 2019

Julow commented Nov 14, 2019 •

edited

Loading