search

bilus / reforms

Beautiful Bootstrap 3 forms for Om, Reagent and Rum.
Eclipse Public License 1.0
168 stars 7 forks source link

Table with row selection: automatically convert to set #6

Open Frozenlock opened 8 years ago

Frozenlock commented 8 years ago

Would it be possible to just coerce any existing data to a set?

To see the problem: Run the demo, but with an initial value of an empty vector, instead of nil.

https://github.com/bilus/reagent-reforms/blob/master/examples/controls/src/controls.cljs#L10

bilus commented 8 years ago

Thanks a lot for the suggestion.

I'm not quite sure I'd like to complicate the semantics by supporting this but I'm definitely not ruling that out. Is there a reason you prefer a vector instead of a set in your particular application?

Frozenlock commented 8 years ago

Yes; Many DB will automatically convert sets into other kinds of collection.

In my case, when I store sets, I get back vectors. It's usually not a problem, because Clojure is built to be able to handles different kind of collections with the same basic functions.

Unfortunately, reforms doesn't like vectors in this particular situation. This means that data stored in the DBs are incompatible with it.

There's also the case where the user generates the :selected field initial values with a map or filter function. These will return collections which are not sets. If reforms doesn't support them, the user have to convert them manually.

Now, I can understand why you'd want to let this step to the user. However, if reforms can't accept these collections, I think it should warn the user: "The :selected field can only be nil or a set."

bilus commented 8 years ago

A set is perfect in this case, esp. for large tables. With a vector we have O(n^2). I would be wary of making it too easy to use a vector because rendering could get sluggish in some cases and the poor user would be none the wiser.

Leaving that aside, it would be possible to create a protocol and make it work for vectors, lists and sets. At the moment I simply have no free time on my hands to do it but if you submit a PR -- fantastic.

BTW. I updated the API reference, it'll go live with the next release -- thank you!