opam
opam copied to clipboard
ocaml
OpamStd.Map.update has a slightly confusing signature and is less powerful than the stdlib one
OpamStd.Map defines its own update function. This was totally justified back when there was no such function in the standard library Map functor but probably is not anymore.
The signature and the documentation made me assume it would behave differently than it does, here is the doc:
(** [update k f zero map] updates the binding of [k] in [map] using function
[f], applied to the current value bound to [k] or [zero] if none *)
val update: key -> ('a -> 'a) -> 'a -> 'a t -> 'a t
I thought zero would be used as binding directly rather than passed to f. Reading the documentation again with that in mind, it is correct but I naturally assumed from the signature it would use zero directly.
This seems a bit unfriendly as not all types can define a meaningful zero, e.g. list has [], option has None but some user defined type might not and working with this is a bit painful.
The stdlib update is much more powerful, it can delete existing bindings in addition to modifying them or adding new ones and has a much better and user friendly API.
I totally understand why OpamStd.Map.update was defined the way it is but I think it's time to let it go.
I also understand you might not want to remove it altogether for compat reasons so maybe re-exposing the stdlib update under a different name so it can be used instead would also be a satisfactory solution here.
I'm happy to submit a PR if you agree that would be useful and we work out the details of how you'd like to do it!
It's probably important for the follow up discussions: the stdlib Map.update was introduced in 4.06.
Looking at it, opam-core requires OCaml >= 4.06, except on windows where it requires 4.03. If this can't be changed it will likely be a bit problematic but we can workout a compat layer for older compilers, re-implementing the stdlib update, in a less efficient fashion obviously.
Gentle ping! Do the maintainers have any thoughts on this? If you agree that this needs fixing, I'd love to discuss what's your prefered solution and then I can implement it and submit a PR!
opam-core requires OCaml >= 4.06, except on windows where it requires 4.03
It’s actually the opposite ^^
In my personal opinion I think if you provide a PR to change OpamStd.Map.update to match the signature of Stdlib.Map.update it would be fine to get it in 2.2.0.
The function isn’t heavily used (only 18 uses as far as i can see) and the Stdlib version is indeed more powerful. It would need to be reimplemented though (to keep the OCaml 4.03 compatibility) but that’s not too hard I don’t think
@NathanReb - are you likely to be able to work on this in the next few months (just looking at milestones)?
