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

feature: show number of errors when waiting in watch mode #8408

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Aug 16, 2023

The goal of this feature is to display to the user the number of errors when idling in watch mode. This can be useful especially when a user has their console history, making it easier to understand how many failures are actually present.

This displays:

Had 4 errors, waiting for filesystem changes...

When the watchmode build has been finished with 4 errors. For a single error it say "Had 1 error".

  • changelog
  • update tests
  • RPC versioning

Sorry, something went wrong.

@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch 3 times, most recently from 132fa29 to 0b93ecc Compare August 16, 2023 14:30
@Alizter Alizter marked this pull request as ready for review August 16, 2023 14:56
@Alizter Alizter requested a review from rgrinberg August 16, 2023 14:56
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 16, 2023

cc @pmwhite this may be useful for you.

@rgrinberg
Copy link
Member

We can do better in general by threading the correct information through the build system.

Why is it better?

@Alizter
Copy link
Collaborator Author

Alizter commented Aug 16, 2023

@rgrinberg Because it makes the information available more widely than just through RPC. Before if I wanted to know how many jobs had failed when updating the status line in bin/import.ml there wasn't anything to use. Now we have a dedicated way of getting the value, which we can use in monitor if we want to or count like I was already doing.

The point is not that it makes life better for dune monitor but rather that it was inspired by dune monitor. I think having this behaviour is useful for users.

@rgrinberg
Copy link
Member

If this information is useful (I'm skeptical of that), it can already be derived from the existing machinery. Just get it from Build_system.errors.

@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch from 0b93ecc to 6f573d2 Compare August 17, 2023 01:24
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 17, 2023

@rgrinberg Thanks, I've dropped the other changes to the engine now and am able to get it directly from Build_system.errors.

@rgrinberg
Copy link
Member

Please revert all the changes to RPC. The number of errors can already be tracked by subscribing to them.

@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch from 6f573d2 to 8e35678 Compare August 17, 2023 14:06
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 17, 2023

@rgrinberg All RPC changes reverted.

@rgrinberg
Copy link
Member

I don't see this change as particularly useful, but I'm not strongly against it either. @snowleopard what do you think?

@pmwhite
Copy link
Collaborator

pmwhite commented Aug 17, 2023

As another data point to weigh, I think I would personally enjoy seeing the current error count. For instance, if I change a type that produces a to-do list of, say, 30 type errors, it'd be nice to see how that number ticks down as I fix each error.

@Alizter
Copy link
Collaborator Author

Alizter commented Aug 18, 2023

Another use case that I had in mind was for when people run tests. From experience porting large test suites to Dune, a commonly asked for feature was a count for how many tests failed. Although that might not be something immediately useful to everybody I think it can be useful to some. Note that VS Code users already have a similar feature by inspecting their "problems" tab.

@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch from 8e35678 to 0d4a622 Compare August 19, 2023 12:36
Copy link
Collaborator

@snowleopard snowleopard 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 to me, though maybe we should add a test to see the "Had 2 errors" message as well?

@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch from 0d4a622 to e57488a Compare August 19, 2023 15:18
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 19, 2023

@snowleopard done.

@snowleopard
Copy link
Collaborator

@snowleopard done.

Thanks! It's not clear to me if the test you added is deterministic though (maybe we want to add some sorting or filtering).


$ . ./helpers.sh

$ export PATH=.:$PATH
Copy link
Member

Choose a reason for hiding this comment

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

Why do you modify the PATH?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was copied from a test, I'll remove it.

@rgrinberg rgrinberg added this to the 3.11.0 milestone Aug 20, 2023

Verified

This commit was signed with the committer’s verified signature.
Alizter Ali Caglayan
This displays:

Had 4 errors, waiting for filesystem changes...

When the watch mode build has been finished with 4 errors. For a single
error it say "Had 1 error".

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch from e57488a to 1f4f71d Compare August 20, 2023 22:47
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 21, 2023

@snowleopard I think the test is deterministic, more rather I don't see why it wouldn't be.

@snowleopard
Copy link
Collaborator

@snowleopard I think the test is deterministic, more rather I don't see why it wouldn't be.

Because one error is racing with another?

@rgrinberg
Copy link
Member

Note that we rely on the order of error messages in many different places in our test suite. This PR isn't making it worse.

@rgrinberg rgrinberg merged commit 0a3b251 into ocaml:main Aug 21, 2023
@Alizter Alizter deleted the ps/branch/feature__show_number_of_errors_when_waiting_in_watch_mode branch August 21, 2023 10:55
@jchavarri jchavarri mentioned this pull request Sep 6, 2023
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 14, 2023
CHANGES:

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)- Use `posix_spawn` instead of `fork` on MacOS. This gives us a
  performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)-
  Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)- Ignore internal promote rules
  when `--ignore-promoted-rules` is set (ocaml/dune#8518, fix ocaml/dune#8417, @rgrinberg)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package
  names. (ocaml/dune#8331, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 22, 2023
CHANGES:

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)

- Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance
  boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)

- Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon)

- Add a new alias `@doc-json` to build odoc documentation in JSON format. This
  output can be consumed by external tools. (ocaml/dune#8178, @emillon)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package names.
  (ocaml/dune#8331, @emillon)

- init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)

- Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance
  boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)

- Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon)

- Add a new alias `@doc-json` to build odoc documentation in JSON format. This
  output can be consumed by external tools. (ocaml/dune#8178, @emillon)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package names.
  (ocaml/dune#8331, @emillon)

- init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
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.

None yet

4 participants