Skip to content

Make merlin libraries public and wrapped#1455

Merged
voodoos merged 2 commits intoocaml:masterfrom
tmattio:public-libs
Apr 12, 2022
Merged

Make merlin libraries public and wrapped#1455
voodoos merged 2 commits intoocaml:masterfrom
tmattio:public-libs

Conversation

@tmattio
Copy link
Copy Markdown
Contributor

@tmattio tmattio commented Apr 11, 2022

Alternative to #1448 and #1172 to make the libraries public and wrap them under a single top-level module.

@tmattio tmattio changed the title Make merlin libraries public and unwrapped Make merlin libraries public and wrapped Apr 11, 2022
@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Apr 11, 2022

This PR is still missing a few that are used by at least ocaml-lsp:

  • query_protocol
  • query_commands

@rgrinberg do you see anything else missing ?

@trefis any thoughts about this ?

@tmattio
Copy link
Copy Markdown
Contributor Author

tmattio commented Apr 11, 2022

This PR is still missing a few that are used by at least ocaml-lsp:

Sorry for the oversight, I added the missing public_names in 3c2e994

@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Apr 11, 2022

Also I think we should document somewhere that we don't provide any guarantee on the stability of the publicly available libraries. Maybe somewhere in the readme ?

@rgrinberg
Copy link
Copy Markdown
Member

do you see anything else missing ?

No, but I also don't see how this PR is better than my original PR.

@tmattio
Copy link
Copy Markdown
Contributor Author

tmattio commented Apr 11, 2022

how this PR is better than my original PR

It's probably not, actually looking a the diff, this looks very similar 🙂

It differs from your PR by:

  • Making the libraries public and wrapping the libraries at the same time
  • Changing the public names to merlin.ocaml_* for everything that is copied from OCaml (e.g. merlin.ocaml_utils)
  • Rebasing on top of master
  • Fixing some dependency cycles in the test suite to make the CI pass

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Apr 11, 2022 via email

@voodoos
Copy link
Copy Markdown
Collaborator

voodoos commented Apr 11, 2022

@rgrinberg the main thing is that this PR finally introduces public_names which is mostly an upstream of rgrinberg@e077c9b

Making Merlin libraries public has been in the backlog for quite some time now.. simply adding public names is not as satisfactory as having separate packages but we decided that it will do for now.

@tmattio proposed this PR because he had it already done for another project that vendors Merlin. It does overlaps yours but we can still merge them successively if you prefer.

@tmattio
Copy link
Copy Markdown
Contributor Author

tmattio commented Apr 11, 2022

It does overlaps yours but we can still merge them successively if you prefer.

I've added @rgrinberg as a co-author of the PR. I don't see much value in merging the other PR first since it's also implemented in the present PR, which is already rebased on master and fixes the CI, but I'll go with whatever you prefer 🙂

@rgrinberg
Copy link
Copy Markdown
Member

It's more of an issue because I'm already using commits from my branch in LSP so it would create extra surgery work.

@tmattio
Copy link
Copy Markdown
Contributor Author

tmattio commented Apr 11, 2022

It's more of an issue because I'm already using commits from my branch in LSP so it would create extra surgery work.

Gotcha! Seems like @voodoos merged your branch, so I'll rebase this PR 🙂

Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @tmattio!

@voodoos voodoos merged commit 4b8acad into ocaml:master Apr 12, 2022
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jun 30, 2022
CHANGES:

Thu Jun 30 14:51:42 CEST 2022

  + merlin binary
    - make most library public and split merlin in two packages: the
      `merlin-lib` package that exposes merlin's internals and the `merlin`
      package with the frontend. (ocaml/merlin#1448, ocaml/merlin#1455, ocaml/merlin#1457, ocaml/merlin#1497, @rgrinberg,
      @tmattio, @kit-ty-kate)
    - Type printing: use best_module_path for paths from Mty_alias (ocaml/merlin#1470)
    - Attempt at finding the 'real' capitalization of files on windows (ocaml/merlin#1462 by
      @mlasson)
    - Use newer `Seq`-based API of Yojson 2.0, avoiding the need for the
      deprecated `Stream` module (ocaml/merlin#1475 by @Leonidas-from-XIV)
    - unify parsing of `MERLIN_LOG` (ocaml/merlin#1480 by @ulugbekna)
    - Fix type deduplication in `type-enclosing` results (ocaml/merlin#1483, fixes ocaml/merlin#1477)
    - Only weakly reduce the shapes to speed up the new Merlin locate
      implementation. (ocaml/merlin#1488)
    - Ignore unknown configuration tags from dune configuration provider but not
      from dot-merlin-reader (ocaml/merlin#1486)
    - typing recovery: recover at the granularity of `core_type` (ocaml/merlin#1484)
  + editor modes
    - add method imenu items for emacs (ocaml/merlin#1481, @mndrix)
    - emacs: Make the prefix argument to `merlin-locate` optional, both for
      consistency with Emacs convention and for backwards compatibility. (ocaml/merlin#1476,
      @antalsz)
    - emacs: fix duplicated prefix path in imenu entries (ocaml/merlin#1495, @bcc32)
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.

3 participants