search

mpdairy / posh

A luxuriously simple and powerful way to make front-ends with DataScript and Reagent in Clojure.
Eclipse Public License 1.0
460 stars 45 forks source link

Fix plugin-base/safe-pull so it checks for number? instead of integer? #34

Closed jpmonettas closed 3 years ago

jpmonettas commented 5 years ago

Changed the id check in safe-pull to check for number? instead of integer? so it can be used with ids like 5.036617769192117e+47

metasoarous commented 3 years ago

Hi @jpmonettas. Thanks for submitting this.

Can you please clarify why this is necessary or useful?

Thanks!

jpmonettas commented 3 years ago

Hi @metasoarous, if I don't remember wrong I was running some experiments where it was useful to use some external generated ids as entities ids. This external generated ids where big integers like 5.036617769192117e+47 but posh was failing because it was only allowing integer? ids and :

(integer? 5.036617769192117e+47) => false (number? 5.036617769192117e+47) => true

Since having number entities ids like that one is possible in Datascript, why should posh limit it to a subset of them? Shouldn't posh support the same entities ids supported by Datascript?

metasoarous commented 3 years ago

I see. Thanks for clarifying @jpmonettas.

My guess is that this is a pretty simple fix somewhere; probably a either a :pre statement or a spec somewhat that's failing to check out. With a full stacktrace, it should be pretty easy to figure out where posh is making this assumption.

That having been said, I'm suspicious that there may be a performance impact associated with using non-integer numbers, so potentially something to be aware of.

Thanks again

metasoarous commented 3 years ago

@jpmonettas Sorry; Thought I was reading an issue. You already have a PR!

My only concern with this is that it's using number?, which would likely let floating point numbers pass through, which may break datascript. If this is addressed, and if you merge the latest from master in to your PR, I can go ahead and update.

Thanks!

jpmonettas commented 3 years ago

@metasoarous hey np, just rebased on top of master.

Cheers!

metasoarous commented 3 years ago

@jpmonettas Great; Thanks for that!

Can you please verify that the type of the values your using as ids here?

Thanks again!

metasoarous commented 3 years ago

Hey @jpmonettas. I realized I should really be directing you to https://github.com/denistakeda/posh, since @denistakeda has taken over maintenance of posh.

Thanks again for raising this issue!