New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpamStd.Map.update has a slightly confusing signature and is less powerful than the stdlib one #4915
Comments
It's probably important for the follow up discussions: the stdlib 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! |
It’s actually the opposite ^^ In my personal opinion I think if you provide a PR to change |
Ok, I'll look into it! |
@NathanReb - are you likely to be able to work on this in the next few months (just looking at milestones)? |
Sure thing! |
OpamStd.Map
defines its ownupdate
function. This was totally justified back when there was no such function in the standard libraryMap
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:
I thought
zero
would be used as binding directly rather than passed tof
. Reading the documentation again with that in mind, it is correct but I naturally assumed from the signature it would usezero
directly.This seems a bit unfriendly as not all types can define a meaningful
zero
, e.g. list has[]
, option hasNone
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!
The text was updated successfully, but these errors were encountered: