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.

Sign up for GitHub

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

Jump to bottom

split describe into cmdliner subcommands #7919

Merged
merged 38 commits into from
Jun 12, 2023
Merged

split describe into cmdliner subcommands #7919

merged 38 commits into from
Jun 12, 2023

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jun 8, 2023 •
edited

As we discussed, this PR splits dune describe into subcommands in preparation for introducing the dune show command.

I've also taken this opportunity to do some misc refactoring in between, hopefully whilst preserving the functionality.

Can be squashed later, I've kept the commits separate in case I have messed anything up and I need to bisect.

  • changelog (it is somewhat user facing)
  • update docs?
  • check --help has not broken and makes sense.

@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from 54f82bd to c12e043 Compare June 8, 2023 19:39
@Alizter Alizter marked this pull request as ready for review June 8, 2023 19:41
@Alizter Alizter requested a review from rgrinberg June 8, 2023 19:42
@Alizter Alizter mentioned this pull request Jun 8, 2023
Merged
@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch 2 times, most recently from 8e6a953 to cadd70b Compare June 8, 2023 20:13
@Alizter Alizter marked this pull request as draft June 9, 2023 09:32
Alizter
Alizter commented Jun 9, 2023
View reviewed changes
test/blackbox-tests/test-cases/describe/describe.t
$ dune describe --lang 0.1 workspac
Error: Unknown constructor workspac
Hint: did you mean workspace?
$ dune describe workspacw --lang 0.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test would actually succeed since cmdliner can infer that workspac -> workspace. Therefore I changed it to actually be incorrect.

@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from cadd70b to 50f3035 Compare June 9, 2023 11:28
@Alizter Alizter marked this pull request as ready for review June 9, 2023 11:29
@Alizter Alizter requested a review from rgrinberg June 9, 2023 11:30
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 9, 2023 •
edited

Actually, I've just noticed that I can really get rid of describe_common. For some reason before, I thought it was used by multiple. I'll do that now.

@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from 50f3035 to 6c71c25 Compare June 9, 2023 12:32
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 9, 2023

OK I've gotten rid of describe_common. There is a describe_format library left which is used by multiple commands. Since this is only about writing dyn as a csexp or sexp it might be worth including that elsewhere.

@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from 6c71c25 to 1325870 Compare June 9, 2023 12:39
@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch 2 times, most recently from fadbce8 to 30418cb Compare June 9, 2023 14:13
@Alizter Alizter requested a review from rgrinberg June 9, 2023 14:29
@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from 30418cb to a99f7f2 Compare June 9, 2023 14:54
@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from a99f7f2 to def9d80 Compare June 10, 2023 11:16
@Alizter Alizter requested a review from rgrinberg June 10, 2023 16:51
@Alizter
refactor: move bin/describe.ml to bin/describe/
511af11 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor: remove indentation in describe
a0b6193 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor: What.describe takes ()
b1e1112 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
test: move dune describe tests to describe/
5089adf 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor: split dune describe into subcommands
e21f268 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move describe external_lib_deps to own file
d80c667 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move describe workspace into own file
529b5e5 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
doc: fix help documentation for dune describe
1852fc3 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move describe opam-files code out of describe_common
8d2fd15 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): tighten up Describe_common.Format API
b105109 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): untangle describe pp from describe_common
a2b6c3a 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): untangle describe external-lib-deps from d_common
8bcf807 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move test sanitation to describe_workspace
01abafd 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move sanitation variable to workspace
081752e 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): move workspace crawling into describe_workspace
066b218 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): misc refactoring of describe_workspace
347b06f 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refator(describe): move comment about command to correct place
141245a 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe_external_lib_deps): small refactorings
f9d414f 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe_opam_files): remove indentation
6837c45 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: add comments to describe_common
b17957d 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
refactor(describe): remove describe_common lib
1400e36 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: add comments to describe_format
4c65cd4 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: add --lang back into describe_opam_files for backcompat
0b051f0 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: add --lang back into describe_pp for backcompat
44672bb 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: add --lang back into external_lib_deps for backcompat
43ce926 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
describe: move dummy --lang argument into own module
4d5a2b4 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
doc(describe): add changelog for command split
96a54ff 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch from 7447fe0 to 96a54ff Compare June 11, 2023 11:19
@rgrinberg
_
9f9c8eb 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit 9ba9901 into ocaml:main Jun 12, 2023
21 of 22 checks passed
@Alizter Alizter deleted the ps/branch/refactor__move_bin_describe_ml_to_bin_describe_ branch June 12, 2023 13:10
@emillon emillon mentioned this pull request Jun 23, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 23, 2023
@emillon
[new release] dune (15 packages) (3.9.0~alpha1)
e84760b 
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
@emillon emillon mentioned this pull request Jun 28, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 28, 2023
@emillon
[new release] dune (15 packages) (3.9.0)
2e2aa14 
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Add `--all` option to `dune rpc status` to show all Dune RPC servers running.
  (ocaml/dune#8011, fix ocaml/dune#7902, @Alizter)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Alizter @rgrinberg