ocaml-batteries-team / batteries-included

Batteries Included project
http://ocaml-batteries-team.github.com/batteries-included/hdoc2/
Other
517 stars 106 forks source link

List not fully imported/replicated into BatList #204

Closed vincent-hugot closed 12 years ago

vincent-hugot commented 12 years ago

Hello,

According to http://ocaml-batteries-team.github.com/batteries-included/hdoc/BatList.html, there is no fold_left function, although there is a fold_right & a fold_left2.

There should be a fold_left function, if only for the sake of consistency and compatibility with the OCaml std lib. Furthermore it is mentioned in the ocamldoc of reduce.

gasche commented 12 years ago

This is a much smaller problem than you think. The idea is that BatList (or rather, ExtList, in batteries ancestor extlib) was not designed to be a stand-alone module, but to be included on top of the standard library, adding functions to an existing List module, shadowing previous definitions if necessary (to get tail-recursive ones).

The usual mode of use of BatList is not BatList.foo, but rather:

open Batteries
... List.foo ...

In this context, fold_left is not missing.

Later it was decided that the use case of calling BatFoo directly without opening Batteries is a reasonable one, and we should import all the definitions of the corresponding "stdlib" module, even those we don't override/change. This hasn't been done consistently for all Batteries module yet, and is indeed a slight defect of the current state, just not as serious as you say.

If you are willing to help us bridge the gap by importing those missing functions into BatList and other similar modules, just send a patch! It's a good way to contribute to Batteries. The idea is simply to add to BatList.mli

(* imported from INRIA's stdlib *)
val fold_left : ...
(** <documentation comment from the compiler lib> *)

and to BatList.ml

let fold_left = List.fold_left
(**T fold_left
   <some unit tests specifying the behavior> **)

(The reason why we didn't do that initially is to avoid the duplication, eg. of documentation comments. If we copy-paste comments from INRIA's stdlib we must, if only for copyright reasons, mention provenance. There will be some maintenance costs associated, that is mirroring possible changes to the documentation on INRIA's side. Also, I insist on adding tests; it's not necessary but desirable for regression testing: if we later wish to change an implementation, we'll be happy to have thorough tests in place.)

vincent-hugot commented 12 years ago

Okay, thanks for the explanation -- I imagined something along those lines (I've been keeping an hopeful eye on Batteries for a while, but I've yet to actually use it).

Beyond the "it's a valid use-case" argument, there is the usability of the documentation: regardless of other considerations it doesn't do to have functions that belong together documented in totally different places.

If you are willing to help us bridge the gap by importing those missing functions into BatList and other similar modules, just send a patch! It's a good way to contribute to Batteries. The idea ...

Okay, I'll do that, at least for BatList; it gives me an excuse to try out Batteries and git...

thelema commented 12 years ago

@gasche when was it decided that we should import anything from stdlib into BatFoo? AFAIK, the only cases where batFoo re-exports stdlib functions withoout change is when we can't do the include Foo include BatFoo trick because of functors or type declarations. (nevermind that at least type declarations aren't a problem as of 3.12).

I've been thinking about cut the ties with stdlib and finish what you started with batMap - having local implementations of all stdlib functionality. We'd still keep compatibility. But then I realize there's no good reason to re-implement all that code. I still haven't convinced myself that it's worthwhile to import all the stdlib declarations.

I can understand the documentation duplication issue, but it doesn't seem too bad to me to look in two places (with links to stdlib Foo from every BatFoo help file).

vincent-hugot commented 12 years ago

[edit for clarity: I'm referring to the documentation problem]

My two cents, from the viewpoint of an OCaml user wondering whether to seriously commit to Batteries...

the project is advertised as a

consistent, documented, and comprehensive development platform

If it were an add-on to the stdlib, it would make sense to ask users to know and care a little bit what is in stdlib and what is not. But if it is a platform, the user should not have to know or care about that kind of historical details. A platform is supposed to stand on its own -- even if, internally, it happens to be a superset of something.

Because if it is an add-on, then the user is supposed to have some working knowledge of the stdlib, and so it's no big deal if you don't find some function in BatFoo, you have the immediate reflex to look in Foo, because you understand or guess what's going on.

But a new user, say, from the Haskell world, downloading the Batteries Platform, and expecting to find a more complete, more friendly Caml environment, will be thrown off and greatly inconvenienced by that, IMHO. (I know I was thrown off by that just reading the doc, that's why I posted... it just makes a bad first impression)

rixed commented 12 years ago

I agree that the documentation of all functions available from batteries (from stdlib or not) should be documented in one place as if everything was defined in batteries, and that it's one of the serious issue with batteries right now that the documentation is not exhaustive and require frequent lookups to stdlib documentation (and only if he's lucky will he find working html links from the one to the other). Having said that, I find it weird to change the code to fix a documentation issue. It would be nice if ocamldoc was run on the "expanded" Thing.mli (the one you get after opening Batteries) instead of the "restricted one" BatThing.mli ...

thelema commented 12 years ago

I'd like to put this to the batteries developers - should we aim to be a "platform" or an "add-on"?

E.

On Fri, Dec 30, 2011 at 10:12 AM, Vincent Hugot < reply@reply.github.com

wrote:

My two cents, from the viewpoint of an OCaml user wondering whether to seriously commit to Batteries...

the project is advertised as a

consistent, documented, and comprehensive development platform

If it were an add-on to the stdlib, it would make sense to ask users to know and care a little bit what is in stdlib and what is not. But if it is a platform, the user should not have to know or care about that kind of historical details. A platform is supposed to stand on its own -- even if, internally, it happens to be a superset of something.

Because if it is an add-on, then the user is supposed to have some working knowledge of the stdlib, and so it's no big deal if you don't find you function in BatFoo, you have the immediate reflex to look in Foo, because you understand or guess what's going on.

But a new user, say, from the Haskell world, downloading the Batteries Platform, and expecting to find a more complete, more friendly Caml environment, will be thrown off and greatly inconvenienced by that, IMHO.


Reply to this email directly or view it on GitHub:

https://github.com/ocaml-batteries-team/batteries-included/issues/204#issuecomment-3313869

thelema commented 12 years ago

On Fri, Dec 30, 2011 at 11:45 AM, Cedric Cellier < reply@reply.github.com

wrote:

I agree that the documentation of all functions available from batteries (from stdlib or not) should be documented in one place as if everything was defined in batteries, and that it's one of the serious issue with batteries right now that the documentation is not exhaustive and require frequent lookups to stdlib documentation (and only if he's lucky will he find working html links from the one to the other).

Does this documentation make clearer the references to stdlib? http://ocaml-batteries-team.github.com/batteries-included/hdoc2/

As to missing references to the stdlib module - Scanf,Str,Unix don't have links to stdlib, and probably should. Every other module that extends a stdlib module (without replacing it entirely) has a link. Am I missing any others?

Having said that, I find it weird to change the code to fix a documentation

issue. It would be nice if ocamldoc was run on the "expanded" Thing.mli (the one you get after opening Batteries) instead of the "restricted one" BatThing.mli ...

I wish it were this simple - to ease our maintenance burden, there's no Thing.mli - all the merging happens in batteries.ml, which has no .mli. This means that we don't have to preprocess our source code when ocaml stdlib adds a new function (to detect the version of stdlib and document the function or not). Please fix what's broken for you, we're very liberal in accepting patches.

E

rixed commented 12 years ago

Yes this new organisation of the manual makes clearer the distinction from stdlib and helps to understand how batteries is designed. Yet my point, as an "platformist" ;), was to say that it should not be required to refer back to stdlib's doc.

As to missing references to the stdlib module - Scanf,Str,Unix don't have links to stdlib, and probably should. Every other module that extends a stdlib module (without replacing it entirely) has a link. Am I missing any others?

Unix and Str are the most stricking since Scanf seams to have most of stdlib content (up to the example). It's hard to tell for the others. Is BatNum, for instance, replacing completely stdlib's Num? It seams so but it's hard to tell from the doc only.

BTW, while browsing through the pages I noticed some css style was setting some font size to 4px (for super/subscript) which can barely be seen. Will patch it someday.

gasche commented 12 years ago

I suggest we move to discuss it on the mailing-list; Github issues are a bit too short-lived for this kind of decision taking.

vincent-hugot, I have cc.ed you in the batteries-devel@lists.forge.ocamlcore.org thread, please complain if you haven't got it, can't post or whatever.

UnixJunkie commented 11 years ago

Still no fold_left in BatList: I don't understand how this issue could have been closed!

opam list batteries

Available packages for 4.01.0dev+trunk: batteries 1.5.0 Community-maintained foundation library

thelema commented 11 years ago

Batteries 2.0 has been marked incompatible with trunk ocaml in opam. Submit a bug report to fix this and you can have batteries2 with your current compiler. On May 20, 2013 1:39 AM, "Francois Berenger" notifications@github.com wrote:

Still no fold_left in BatList: I don't understand how this issue could have been closed! opam list batteries

Available packages for 4.01.0dev+trunk: batteries 1.5.0 Community-maintained foundation library

— Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/204#issuecomment-18133018 .

UnixJunkie commented 11 years ago

I see this in the OPAM package for batteries.2.0.0: ocaml-version: [<= "4.00.1"] There is no such thing in the 1.5.0 package description.

Is there something that should be fixed?

thelema commented 11 years ago

I was hoping anil would chime in about now, as he's the one that added that restriction. CCing him on this email; maybe he recalls what failed.

On Mon, May 20, 2013 at 8:51 PM, Francois Berenger <notifications@github.com

wrote:

I see this in the OPAM package for batteries.2.0.0: ocaml-version: [<= "4.00.1"] There is no such thing in the 1.5.0 package description.

Is there something that should be fixed?

— Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/204#issuecomment-18182955 .