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

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

Merged
merged 10 commits into from
Jun 1, 2023
Merged

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

merged 10 commits into from
Jun 1, 2023

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented May 18, 2023 •
edited
Loading

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
nojb
nojb reviewed May 18, 2023
View reviewed changes
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 gpetiot force-pushed the optional-dialect-intf branch from e055782 to 468ec27 Compare May 19, 2023 10:26
@gpetiot
Copy link
Contributor Author

gpetiot commented May 19, 2023

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

nojb
nojb reviewed May 22, 2023
View reviewed changes
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.

emillon
emillon requested changes May 22, 2023
View reviewed changes
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
@nojb nojb mentioned this pull request May 22, 2023
Closed
@emillon emillon linked an issue May 22, 2023 that may be closed by this pull request
Make (interface) optional in (dialect) #7776
Closed
@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
nojb
nojb reviewed May 27, 2023
View reviewed changes
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 gpetiot force-pushed the optional-dialect-intf branch from 36efcb6 to 4d3336f Compare May 30, 2023 08:46
@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.

@gpetiot
Make the 'interface' field of a dialect optional
3b5fb99 
Signed-off-by: Guillaume Petiot <guillaume@tarides.com>
@gpetiot gpetiot force-pushed the optional-dialect-intf branch from 4d3336f to 3b5fb99 Compare May 30, 2023 16:57
@gpetiot gpetiot requested a review from nojb May 30, 2023 16:58
nojb
nojb approved these changes May 31, 2023
View reviewed changes
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
@gpetiot
Update the manual and the changelog
9143ccf 
Signed-off-by: Guillaume Petiot <guillaume@tarides.com>
@gpetiot gpetiot requested a review from christinerose as a code owner May 31, 2023 11:03
Alizter
Alizter reviewed May 31, 2023
View reviewed changes
doc/dune-files.rst Outdated Show resolved Hide resolved
gpetiot added 2 commits May 31, 2023 14:46
@gpetiot
Update the manual for dialects
cfa4438 
Signed-off-by: Guillaume Petiot <guillaume@tarides.com>
@gpetiot
Merge branch 'main' into optional-dialect-intf
3e7136a
@nojb
Copy link
Collaborator

nojb commented Jun 1, 2023

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

emillon
emillon reviewed Jun 1, 2023
View reviewed changes
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.

@emillon
Apply suggestions from code review
ef39620 
Signed-off-by: Etienne Millon <etienne.millon@gmail.com>
nojb and others added 3 commits June 1, 2023 16:23
@nojb
Merge branch 'main' into optional-dialect-intf
ed02304
@nojb @christinerose
Update test/blackbox-tests/test-cases/dialects/no_impl.t/run.t
a449904 
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb @christinerose
Update test/blackbox-tests/test-cases/dialects/no_intf_bad.t/run.t
f5179e9 
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 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

@Alizter Alizter Alizter left review comments

@emillon emillon emillon requested changes

@christinerose christinerose christinerose approved these changes

@nojb nojb nojb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.

Make (interface) optional in (dialect)
6 participants
@gpetiot @emillon @nojb @Alizter @christinerose @rgrinberg