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

Make the interface and implementation fields of a dialect optional #7757

Merged
merged 10 commits into from
Jun 1, 2023

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented May 18, 2023

Suggested by @emillon in ocaml/ocaml.org#1181 (comment)

Our usecase is defining a dialect for .eml files, that don't have interface files.

If I had changed the Ml_kind.Dict.intf into an option type there would be too many changes to propagate (I tried) and it makes the code harder to follow (harder to keep the impl/intf abstraction when reading the fields).

@rgrinberg rgrinberg requested a review from nojb May 18, 2023 10:44
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I'm not fond of the dummy extension. The type Ml_kind.Dict is used in many other places apart from being used for dialects. What about defining a type specifically for dialects that has an optional intf?

@gpetiot
Copy link
Contributor Author

gpetiot commented May 19, 2023

I added a new module Ml_kind.Dialect, let me know what you think.

Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I left a few comments. Also, as a general rule, I am not fond of infix operators such as @@, but that's my personal preference. I don't know if the other Dune devs have a preference. cc @rgrinberg @emillon @Alizter

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/ocaml/ml_kind.ml Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator

emillon commented May 22, 2023

Regarding (@@), indeed that's not something we use a lot in the dune code base. But in many cases it looks like you can replace it with other functions, I'll leave some comments.

@emillon
Copy link
Collaborator

emillon commented May 22, 2023

Just thinking out loud: we plan to add some formatting-only dialects (for example if you want to set a formatter for JSON files in a project). We just need to make sure that there's no syntax clash. But I don't think that these are going to use (implementation) or (interface), so this should not interfere.

@nojb
Copy link
Collaborator

nojb commented May 22, 2023

Just thinking out loud:

Indeed what you describe is a different concept that dialects which are meant for "formatters" that outputs OCaml code meant to be compiled.

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
src/dune_rules/merlin/merlin.ml Outdated Show resolved Hide resolved
src/dune_rules/merlin/merlin.ml Outdated Show resolved Hide resolved
src/dune_rules/module.ml Outdated Show resolved Hide resolved
@emillon emillon linked an issue May 22, 2023 that may be closed by this pull request
@gpetiot gpetiot changed the title Make the (interface) field of a (dialect) optional Make the interface field of a dialect optional May 25, 2023
@gpetiot gpetiot force-pushed the optional-dialect-intf branch 5 times, most recently from 12c8a83 to 36efcb6 Compare May 26, 2023 17:09
@gpetiot gpetiot changed the title Make the interface field of a dialect optional Make the interface and implementation fields of a dialect optional May 26, 2023
@gpetiot gpetiot requested review from nojb and emillon May 26, 2023 17:36
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

Just to confirm: when, say, (implementation) is missing, then it is still possible to provide one by using .ml? I would add a test showing this.

src/dune_rules/dialect.ml Outdated Show resolved Hide resolved
@gpetiot
Copy link
Contributor Author

gpetiot commented May 30, 2023

Just to confirm: when, say, (implementation) is missing, then it is still possible to provide one by using .ml? I would add a test showing this.

I added a test and it works as expected.

Signed-off-by: Guillaume Petiot <guillaume@tarides.com>
@gpetiot gpetiot requested a review from nojb May 30, 2023 16:58
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM. A Changes entry and the manual need to be updated.

@rgrinberg rgrinberg added this to the 3.9.0 milestone May 31, 2023
Signed-off-by: Guillaume Petiot <guillaume@tarides.com>
doc/dune-files.rst Outdated Show resolved Hide resolved
@nojb
Copy link
Collaborator

nojb commented Jun 1, 2023

Looks OK to go to me. @emillon does it look OK to you?

doc/dune-files.rst Outdated Show resolved Hide resolved
doc/dune-files.rst Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator

emillon commented Jun 1, 2023

Looks OK to go to me. @emillon does it look OK to you?

Later I think we should just document the latest version of the language (and omit changelog bits from the manual), but we're not completely there yet. In the meantime I think it's better to use directives than prose for this so I pushed some suggestions.

Signed-off-by: Etienne Millon <etienne.millon@gmail.com>
nojb and others added 3 commits June 1, 2023 16:23
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb merged commit aaabac7 into ocaml:main Jun 1, 2023
@gpetiot gpetiot deleted the optional-dialect-intf branch June 19, 2023 09:34
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 23, 2023
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 added a commit to emillon/opam-repository that referenced this pull request Jun 28, 2023
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make (interface) optional in (dialect)
6 participants