melange: interpret melc --where as a list of :-separated paths
#7176
Conversation
anmonteiro
commented
Feb 24, 2023
•
anmonteiro
commented
- This makes editor integration work with melange: build stdlib, runtime and tests with dune melange-re/melange#493
- It can be merged independently, since it works with single paths too
anmonteiro
force-pushed
the
anmonteiro/merlin-melangelib
branch
from
cfd1f55
to
b19396f
Compare
|
On cygwin will this be |
|
That's a good idea. I'll change it |
anmonteiro
force-pushed
the
anmonteiro/merlin-melangelib
branch
from
b19396f
to
8925ad6
Compare
|
Changed to use |
|
I think you'd be better off just using |
Why is that? I think I'd prefer to stick to the |
|
To me, the bigger surprise is varying the path separator depending on the platform. In any case, it's just a suggestion. |
| in | ||
| Option.map | ||
| ~f:(fun dirs -> | ||
| String.split ~on:Bin.path_sep dirs |> List.map ~f:Path.of_string) |
There was a problem hiding this comment.
If this is a relative path, is it clear what it should be relative to?
There was a problem hiding this comment.
Melange interprets it relative to the melc executable. But it shouldn't really be a relative path. That's a fallback mechanism.
There was a problem hiding this comment.
perhaps I should clarify: the result of melc --where is guaranteed to be an absolute path. So I believe dune doesn't have to worry about it being a relative path.
There was a problem hiding this comment.
If it can't be a relative path, perhaps the code shouldn't allow it. E.g. Path.External.of_string.
There was a problem hiding this comment.
Changed to a Path.External.t in 44cf928
anmonteiro
force-pushed
the
anmonteiro/merlin-melangelib
branch
from
44cf928
to
310797c
Compare
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro
force-pushed
the
anmonteiro/merlin-melangelib
branch
from
c29aebb
to
9340ec2
Compare
| Option.map | ||
| ~f:(fun dirs -> | ||
| String.split ~on:Bin.path_sep dirs | ||
| |> List.map ~f:Path.External.of_string) | ||
| melange_dirs | ||
| |> Option.value ~default:[] |
There was a problem hiding this comment.
This would be more readable (imo) and consistent with code style above without Option functions:
match melange_dirs with
| None -> []
| Some dirs ->
String.split ~on:Bin.path_sep dirs
|> List.map ~f:Path.External.of_string
src/dune_rules/merlin/merlin.ml
Outdated
| let+ dirs = Melange_binary.where sctx ~loc:None ~dir in | ||
| match dirs with | ||
| | [] -> (None, []) | ||
| | [ stdlib_dir ] -> (Some (Path.external_ stdlib_dir), []) |
There was a problem hiding this comment.
If we are converting to Path.t here using external_ in this line and 482 below, would it make sense to just return Path.t list Memo.t from Melange_binary.where directly?
There was a problem hiding this comment.
See #7176 (comment)
There was a problem hiding this comment.
Actually, Bin.parse_sep doesn't allow external paths either.
There was a problem hiding this comment.
I changed it to use Bin.parse_sep.
src/dune_rules/merlin/merlin.ml
Outdated
| match dirs with | ||
| | [] -> (None, []) | ||
| | [ stdlib_dir ] -> (Some (Path.external_ stdlib_dir), []) | ||
| | stdlib_dir :: extra_obj_dirs -> |
There was a problem hiding this comment.
Is it a little weird that the first directory is being special cased?
There was a problem hiding this comment.
I agree, and it might not necessarily be the stdlib if we're not careful, I think that might not matter though?
rgrinberg
force-pushed
the
anmonteiro/merlin-melangelib
branch
from
42a390c
to
e6dc5c2
Compare
* main: (56 commits) feature: add terminal ui backend based on NoTTY (ocaml#6996) doc(coq): update documentation about coqdep fix(rules): don't descend into automatic subdirs infinitely (ocaml#7208) benchmark: add warm run (ocaml#7198) test: vendored and public libs (ocaml#7197) test: use sh in concurrent test (ocaml#7205) fix: custom log file path (ocaml#7200) test(melange): add test exercising ocaml#7104 (ocaml#7204) test(melange): add a test that introduces rules in the target dir (ocaml#7196) test: duplicate packages in vendor dir (ocaml#7194) melange: interpret `melc --where` as a list of `:`-separated paths (ocaml#7176) perf: add synthetic benchmark (ocaml#7189) Test case for bug report (ocaml#6725) Add test illustrating ocaml#6575 (ocaml#6576) chore: add rule streaming proposal (ocaml#7195) test(stdlib): merge wrapped/unwrapped tests test: move all stdlib tests fix: allow unwrapped libraries with `(stdlib ..)` test: demonstrate crash in modules.ml when `(stdlib .. )` used with `(wrapped false)` fix(install): respect display options (ocaml#7116) ...
