Split file

[dlesbre]

This splits the rather large file into smaller independent files (int operations/signatures/keys and values/functors), using a toplevel library interface file to ensure doing so does not change the outside interface. It also gets rid of some duplication (no need to have the signatures both in and mli and an ml file)

Otherwise there is no real change happening here, just stuff being moved around.

[mlemerre]

Shouln't there be some deletions? I see only additions in the changes

EDIT: They are hidden as renames into signatures.ml and functors.ml

[mlemerre]

Maybe we should explain what a Node is here shortly?

[mlemerre]

Maybe say that functors are used to instantiate the library

[dlesbre]

Well this is kind of the problem of this approach: where to document the files? Its either above the include, in the toplevel file, or at the top of the file itself... Since generating documentation just copies the Functor.mli file here, its a bit weird to have both.

I have admittedly, used different conventions here (functors is documented in the file, whereas node is documented at the include site). I'll try and unify those.

[mlemerre]

Good idea, we were getting lost among all those huge files! I only have minor comments (feel free to merge when you think it is ready)

[mlemerre]

I think you could use include subdirs in dune or something similar. But probably calling this comment_prelude or something like that would work 11 juil. 2024 18:43:05 Dorian Lesbre ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In src/prelude.ml[#10 (comment)]: > +(* *) +(* You can redistribute it and/or modify it under the terms of the GNU *) +(* Lesser General Public License as published by the Free Software *) +(* Foundation, version 2.1. *) +(* *) +(* It is distributed in the hope that it will be useful, *) +(* but WITHOUT ANY WARRANTY; without even the implied warranty of *) +(* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *) +(* GNU Lesser General Public License for more details. *) +(* *) +(* See the GNU Lesser General Public License version 2.1 *) +(* for more details (enclosed in the file LICENSE). *) +(**************************************************************************) + +(** File run by MDX before running all others, sets up some stuff so the + comments don't have to *) It is only usefull when running mdx yes, but not just for the mld, it is used in all mli files. It used to exist atop the *PatriciaTree.mli* file inside stop comments, so as to not show up in the generated doc. However, having it separate is a bit better IMO: * it avoids duplication * it avoids spam for people who read documentation in mli files directly But yes I can rename it if you thinks that's better (I have actually tried to place it outside of the *src* folder, but dune won't let me...). — Reply to this email directly, view it on GitHub[#10 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AATOY3RXZTZVQLR7N4OSS4DZL2YZTAVCNFSM6AAAAABKWSA6UCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZSGQ2TKMRZHA]. You are receiving this because you commented. [Image de pistage][https://github.com/notifications/beacon/AATOY3SYXURG7JIBTUSAARTZL2YZTA5CNFSM6AAAAABKWSA6UCWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUBPUEYE.gif]

[mlemerre]

Maybe you can have a longer self explaining title, and document inside the included file. Like functors : instantiating the domain, something like that 11 juil. 2024 18:57:36 Dorian Lesbre ***@***.***>:

…

***@***.**** commented on this pull request. ---------------------------------------- In src/PatriciaTree.mli[#10 (comment)]: > + tree, which is log(n) if we assume an even distribution of numbers + in the map (e.g. random distribution, or integers chosen + contiguously using a counter). The worst-case height is + O(min(n,64)) which is actually constant, but not really + informative; log(n) corresponds to the real complexity in usual + distributions. *) + +(** {1 Integer manipulations} *) + +include module type of Ints + +(** {1 Signatures} *) + +include module type of Signatures + +(** {1 Functors} *) Well this is kind of the problem of this approach: where to document the files? Its either above the include, in the toplevel file, or at the top of the file itself... Since generating documentation just copies the *Functor.mli* file here, its a bit weird to have both. I have admittedly, used different conventions here (*functors* is documented in the file, whereas *node* is documented at the include site). I'll try and unify those. — Reply to this email directly, view it on GitHub[#10 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AATOY3U4SKCQBG27V4WVJK3ZL22QBAVCNFSM6AAAAABKWSA6UCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZSGQ4TAOJYGE]. You are receiving this because you commented. [Image de pistage][https://github.com/notifications/beacon/AATOY3S245UCJ5PJVSGFNFDZL22QBA5CNFSM6AAAAABKWSA6UCWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUBPWKOK.gif]

[dlesbre]

I've had an issue when generating documentation: ocaml/odoc#1162.

Basically, masking the Sigs module breaks documentation links and thus forces-inlines all module types in all other modules. This make documentation far less legible (it replaces module MakeMap(Key: KEY) : MAP by module MakeMap(Key: sig ... end) : sig ... end in the HTML.

So I've decided to expose the Sigs module. As a consequence, all module types and some types now have a Sigs prefix added to them.

We could maintain backward compatibility by both copying Sigs and then including it, but that duplicates all signatures and seems a bit too messy for my taste.

[mlemerre]

Changing the code to help the documentation generation feels very wrong, but the new code is not worse than previously, so I guess it is OK...

[jonludlam]

Changing the code to help the documentation generation feels very wrong, but the new code is not worse than previously, so I guess it is OK...

It's really a consequence of the way namespacing works. The problem is that when you chose to hide certain modules/module types/types from your interface, you need to make sure nothing in your exposed interface references those things. OCaml itself doesn't 'protect' you from this because everything is actually still accessible, just under a mangled name. So before the Sigs module was exposed, odoc essentially has to choose between rendering the docs as:

module MakeMap(Key: PatriciaTree__Sigs.KEY) : PatriciaTree__Sigs.MAP

or inlining the signatures and emitting a warning. Admittedly the warning is probably lost in the sea of irrelevant warnings, but we're working on that!

There's another solution (@canonical tags), but it again boils down to ensuring that all relevant signatures are exposed somewhere.

[jonludlam]

To expand on the other solution - it'll be useful if there are only parts of the Sigs module that you want to use in the public interface. E.g. if you only actually cared about the MAP module type, what you could do would be to expose that in the public interface:

(* PatriciaTree.mli *)

module type MAP = Sigs.MAP

and ensure that there's a canonical tag on the module type in the (hidden) Sigs module:

(* Sigs.mli *)

module type MAP = sig
   (** @canonical PatriciaTree.MAP *)
...
end

Then when odoc notices uses of the Sigs.MAP module type, it knows that they should appear in the docs as PatriciaTree.MAP instead. These canonical tags can be put on modules, module types and types.

[dlesbre]

Oh I didn't know about the canonical tag. Thanks for mentioning it.

It seems that using it with an include works fine, it avoids having to expose the module (and thus changing our interface).

[dlesbre]

Also, I've tested it and no warning is emitted when a signature is inlined.

[jonludlam]

Also, I've tested it and no warning is emitted when a signature is inlined.

Ah, thanks for testing. I'll open this as an issue on odoc.