Skip to content

Split Merlin in two (three) packages#1457

Merged
voodoos merged 8 commits intoocaml:masterfrom
voodoos:merlin-lib
Apr 14, 2022
Merged

Split Merlin in two (three) packages#1457
voodoos merged 8 commits intoocaml:masterfrom
voodoos:merlin-lib

Conversation

@voodoos
Copy link
Copy Markdown
Collaborator

@voodoos voodoos commented Apr 12, 2022

Most of the libraries are now publicly avalaible in the merlin-lib package.
This include the protocol: query_commands and query_protocol which are useful to lib users.
We might add Query_json too.

The merlin package contains the frontend.

All version of the packages are synced, but maybe this is not mandatory for do-merlin-reader ?

@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Apr 12, 2022

(tests should fail. I want to check that the CI catch it)

Thanks a lot @kit-ty-kate for helping me sort out all this!

kit-ty-kate
kit-ty-kate reviewed Apr 12, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Apr 12, 2022
View reviewed changes
@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Apr 13, 2022

I removed the dependency on yojson but it came with a cost: the logger does pretty print json in some cases, so I had to add the pretty printing code from yojson to merlin-lib's std.

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Apr 13, 2022

I removed the dependency on yojson but it came with a cost: the logger does pretty print json in some cases, so I had to add the pretty printing code from yojson to merlin-lib's std.

That seems silly to me.
Let's just depend on yojson? The "plateform" people will eventually vendor everything IIUC, no?

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Apr 13, 2022

That seems silly to me.

Indeed. Another alternative is to leave a global reference that contains a Json.t -> string (or formatter) that users of the library can use to add their own encoding. You would think that a library would allow callers to customize logging anyway.

Let's just depend on yojson? The "plateform" people will eventually vendor everything IIUC, no?

In lsp, I would like to experiment with a different json encoding/decoding layer than yojson for efficiency. It's not something I need right away, but i do plan to drop yojson eventually.

@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Apr 13, 2022 •
edited
Loading

Indeed. Another alternative is to leave a global reference that contains a Json.t -> string (or formatter) that users of the library can use to add their own encoding. You would think that a library would allow callers to customize logging anyway.

I tried that in 0b25a95. It is not the cleanest of the apis but it does feels a lot better than the previous solution. What do you think ?

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Apr 13, 2022

Indeed. Another alternative is to leave a global reference that contains a Json.t -> string (or formatter) that users of the library can use to add their own encoding. You would think that a library would allow callers to customize logging anyway.

I tried that in 0b25a95. It is not the cleanest of the apis but it does feels a lot better than the previous solution. What do you think ?

IMO it's preferable to copy-pasting or doing nothing.

trefis
trefis reviewed Apr 14, 2022
View reviewed changes
@voodoos voodoos force-pushed the merlin-lib branch 3 times, most recently from 7061753 to a61acd3 Compare April 14, 2022 13:36
voodoos and others added 2 commits April 14, 2022 10:54
@voodoos @kit-ty-kate
Adapt the CI to the new packages
58b8bbf 
(Setup-ocaml automatically pins the packages)

Co-authored-by: Kate <kit.ty.kate@disroot.org>
@voodoos
Add changelog entry
395fca2
@voodoos voodoos merged commit 0fb973c into ocaml:master Apr 14, 2022
voodoos added a commit to voodoos/opam-repository that referenced this pull request Jun 30, 2022
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.6-414)
e3dfd5a 
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)
@voodoos voodoos mentioned this pull request Jun 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trefis trefis trefis left review comments

@let-def let-def let-def left review comments

+2 more reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@voodoos @trefis @rgrinberg @let-def @kit-ty-kate