search

metabase / toucan

A classy high-level Clojure library for defining application models and retrieving them from a DB
Eclipse Public License 1.0
569 stars 49 forks source link

Make defmodel take protocol implementations #14

Closed plexus closed 7 years ago

plexus commented 7 years ago

Pass implementations of protocols directly to defmodel, the same way you would do in defrecord or deftype. (fixes #9)

Also provides implementations for empty (fixes #1) and apply (fixes #2).

This updates the documentation, promoting this way of doing things as the preferred method (vs using extend...merge...IModelDefaults), but still leaves in an example of how to do things the old-fashioned way.

The defining-models doc has been expanded slightly to further clarify the relationship between User and (User 1).

The fill column in that document was irregular but seemed to hover around 120, so I went with breaking lines at 120 characters. It might be best to ignore whitespace changes when reviewing this.

Note that as it stands you can implement protocols in defmodel, but not interfaces, since the forms are inserted in extend rather than in defrecord itself. To fix this we'll have to separate extensions to IModel from other interface/protocol implementations.

plexus commented 7 years ago

Diff ignoring whitespace: https://github.com/metabase/toucan/pull/14/files?w=1

plexus commented 7 years ago

I'll update VERSION-HISTORY once #13 is merged, or we'll be getting conflicts.

camsaul commented 7 years ago

@plexus sorry for the delay. I'll take a look at this and your other PR

camsaul commented 7 years ago

@plexus don't worry about updating the VERSION-HISTORY, I'll do it when I push out a new release

plexus commented 7 years ago

I cleaned things up that were pointed out in the review. My apologies for making seemingly arbitrary changes, I reverted them. I tend to use emacs' fill-paragraph functionality a lot when writing docs. However in this case it seems the docs are wrapped by hand, so their fill-column isn't consistent.

camsaul commented 7 years ago

@plexus Sorry it took me so long to merge this things have been a little hectic around here. But it looks good. Thanks!