utop
utop copied to clipboard
ocaml-community
#info directive
This PR adds a depopt, ocp-index to utop. If utop is built when ocp-index is installed, a new directive, #infoof, will present.
This directive works similarly to #typeof. The symbol resolving and completion mechanism is based on the current Env of the Toploop. That is, if we type open String;; in utop, #infoof "concat" will still find out the very infomation of String.concat
Click to expand
I have to say that as an English speaker I find "#infoof" amazingly hard to read. (I also wouldn't guess what it does from the name, the first parse that comes to mind is "in foof" and that's pretty incomprehensible...)
Yeah this isn't a great name. #info is fine; #doc is fine; #info_of could work.
Ha, at the first glance. I feel that the name is very funny. foof ʕoٹoʔ ʕ•̀ω•́ʔ
That's a nice feature :) BTW, I would make the dependency on ocp-index be a required one rather than an optional one. Optional dependencies always end up being annoying in the long run.
The implementation of ocp-index seems not stable for now(we have to fork a clean room for it). I'd like to keep it as an optional dependency to observe for a period of time. When everything is stable enough, we then make it as a required dependency. Is this ok?
I have no opinion on the optional vs. required dependency issue.
How do we decide when this is okay to merge?
@diml I've seen negative opinions about optional dependencies, but what is the main objection against them?
@kandu that seems reasonable yeah.
@pmetzger I'd suggest that someone test it on its machine, just to make sure nothing obvious was missed, and eyeball the code to make sure things seem reasonable. For instance that no unwanted dependency was added by accident or a rootkit was slipped in (just joking here, though we should definitely do it for new contributors).
Then I'd say it's good to merge. This looks like a feature that is worth having.
@bluddy the main source of pain is recompilation. When you install a new package, you have to recompile all already installed packages that have a depopts on this package plus all their reverse dependencies. For utop I guess it's probably not too bad as it doesn't have many reverse dependencies.
But in this case there is also something else: whenever you setup a new switch, you now have to remember to install both utop and ocp-index if you want all the features of utop.
It seems to me that requiring ocp-index as mandatory has only one defect, which is that sometimes it is broken when a new release comes out.
yeah, considering about this, to keep it as an optional dependency is a good option since what you've just said implies we are confident that zed, lambda-term and utop will keep the pace with new ocaml releases better than ocp-index will do :) And once that happens, utop will be still installable without the presenting of #info directive temporarily.
This feature is disabled on Windows since Unix.fork is not available on it.
By the way, are there any easy expressions in dune which I can use to define required libraries more flexibly? like:
(libraries
((and (= %{os_type} Unix) (= (system "opam var ocp-index:installed") "true"))
ocp-index))
Only requires ocp-index as dependency when we are on Unix and ocp-index is installed.
@kandu Good question. I don't know. Perhaps you should ask the Dune maintainers?
Not really, though there is a ticket about this.
What about using spawn? It's a portable and more efficient way of spawning sub-processes. Note that it's under the janestreet org, however it is developed on github as a normal open source project, so you can expect the API to be stable.
There are two ways to use ocp-index.
-
to invoke the ocp-index excutable every time when user types #info "something". So we can call Unix.create_process or spawn to invoke it. This works on all OSes, including Windows. But we'll always tolerate the lag, especially when inquiring from a big library like core_kernel.
-
to use the LibIndex library, create and keep the index data permanently in a process memory. Module info is cached in the lazy-evaluation data structure when user re-inquire some info of the same library. But because of the implementation issuse, we have to workaround it, to fork a clean room for it or the CPU will hang. But since Unix.fork is not implemented on Windows, we have to disable this feature on Windows or we have to either create a standalone executable wrapping LibIndex or we add a new cmd-opt to utop to tell it to work in "ocp-index-server" mode and then spawn a child utop to inquire info. We now just simply disable it on Windows.
I think the second way is preferable, ocp-index developers will eventually fix the bug and we will not need to fork a clean room for it and thus it will work on all OSes. The lack of Windows support is temporary.
Hi, @AltGr @Drup The step-to-reproduce is quite obvious, Should I create a new issue in https://github.com/OCamlPro/ocp-index/ ?
I see. Personally, I would recommend to fix ocp-index first. It means that we have to wait before making this feature available to users, but on the other hand when we release it both utop and ocp-index will be in a better state.
Wow, this looks great, thanks! @kandu, if you could open an issue as you proposed, that'd be very helpful and I'll look into it. :)
Ah, thanks! here is the issue report https://github.com/OCamlPro/ocp-index/issues/136
Great feature! Method 2 is clearly preferable, and libindex should be fairly easy to use. Let's try to beat ocp-index into shape so that everything works fine :p
btw, why not show the doc information in the suggestion bar on the bottom? This is what bpython does for example.
BTW, I note this has been stalled out for a long time. What do we need to do to get it moving?
@pmetzger I guess someone has to fix https://github.com/OCamlPro/ocp-index/issues/136
I'm not familiar with the implementation of this PR, but if it just adds a new directive, I don't think it has to be maintained as part as utop. It can be implemented as something that when #require'd, will add the directive. Users can call utop -require ocpindex.top or add this to their dotfiles to do it automatically. Would that work for you?
