Pull requests

#1 Declined
Repository
w01fe w01fe
Branch
default
Repository
kotarak kotarak
Branch
default

Fix bug where .entryAt returns entry when none present, add error-checking to quote-values, and replace force with deref

Author
  1. w01fe
Reviewers
Description

I believe 'find' was buggy on lazyMap when applied to a non-present key. This fixes that issue and also adds an even? check to quote-values, which prevents user bugs with the lazy-hash-map constructor. I also replaced force with deref, which allows another interesting application of lazymap involving futures (and should not affect existing applications).

  • Learn about pull requests

Comments (10)

  1. w01fe author

    Do you have any interest in merging this? It's a very small change that fixes a bug we ran into in real code, and this is blocking us from releasing our own project that depends on lazymap. If not, we'll probably release our own fork, possibly rolled into a larger project release. Please let me know, and thanks for the great library!

  2. Meikel Brandmeyer repo owner

    I'm sorry for the late reply. The last weeks where rather busy. I remember something like this from the past. I will check tomorrow when I'm back at my dev machine. Seems like a reasonable fix.

  3. w01fe author

    No problem, thanks for looking! I also just added another commit that replaces force by deref, which enables an interesting application using futures, in case you have any interest in merging that as well.

  4. w01fe author

    Also wanted to mention one issue I noticed which I don't think is mentioned in the documentation: lazymap is not quite a drop-in replacement for hash-map, because the LazyMapEntries don't have the same equality semantics as IPersistentMap entries:

    user> (= [1 2] (lazymap/lazy-hash-map 1 2)) false user> (= [1 2] (first {1 2})) true

    (As far as I can tell Clojure's semantics here is the odd one, because it means Clojure's map entries don't actually obey the MapEntry contract that they claim to implement. But still potentially a gotcha for lazymap users).

  5. w01fe author

    Looks good, much appreciated! I'll pull lazymap out of plumbing as a proper dep in the next day or two, and let you know if we run into any issues. Cheers, Jason

  6. Meikel Brandmeyer repo owner

    Another BTW: I also added future maps. So lazy-hash-map could also be a future-hash-map. The constructor functions are deprecated, but still in place. However there are now delayed-*-map constructors. So you can also put promises in the map if you like. (If we now have the possibility to get values out of futures, we should also have a way to get futures in.)

  7. w01fe author

    No worries, thanks for merging!

    In case you want to see our application, we're using lazymap as part of Graph (just updated to use your new release as a dependency): https://github.com/Prismatic/plumbing (we're using delay-assoc to get futures in, as a really simple way to auto-parallelize a graph's computation). Thanks again for the great work.