hoplon / demos

Example ClojureScript applications using hoplon
81 stars 27 forks source link

new demo with support for update round trips to Datomic #25

Closed raymcdermott closed 8 years ago

mynomoto commented 8 years ago

I gave you commit rights, use them wisely :) Code inside deftask, defn, defn-, defrpc (anything that starts withdef really) is indented usually with 2 spaces. On the app.datomic-query, demo.api.user, demo.datomic.api is kind of a bad practice not passing the conn to the functions that use that. This makes easier to test the functions.

raymcdermott commented 8 years ago

Thanks. It would still be nice to review the code in some way to uncover these best practise type issues.

I use mount to inject the connection which makes testing straightforward and reduces the boiler plate on the connection.

Are you guys not believers in that approach or is there some other best practise that I'm missing? Obviously happy to conform if you have strong preferences.

Sent from my iPhone

On 03 Jan 2016, at 02:59, Marcelo Nomoto notifications@github.com wrote:

I gave you commit rights, use them wisely :) Code inside deftask, defn, defn-, defrpc (anything that starts withdef really) is indented usually with 2 spaces. On the app.datomic-query, demo.api.user, demo.datomic.api is kind of a bad practice not passing the conn to the functions that use that. This makes easier to test the functions.

— Reply to this email directly or view it on GitHub.

mynomoto commented 8 years ago

While I like mount and think it's better than component for what it does, I don't like the examples on the repo because they rely on global state when they don't need to. You can test the functions if you pass the conn as an argument without need to tinkering with the global state. You can create a datomic db and just use it on tests, they are easier to reason about. The argument were better made on http://blog.jenkster.com/2015/12/what-is-functional-programming.html Also, that's my opinion, and Micha says that there is no such thing as the right way so if you think your way is better after thinking about it, go for it ;) And feel free to open PRs if you want comments, but not everything needs it.

tolitius commented 8 years ago

@mynomoto, I would love to have better examples in mount. what would be a good example in your opinion? Do you mean that these guy(s) can/should take DB connection as an argument vs. relying on a top level db state?

feel free to open an issue, or ping me on #mount clojurians slack to discuss further.

raymcdermott commented 8 years ago

Thanks for the reference and he makes the points well. I had a good think and a chat with some colleagues in work today to review the pros and cons.

On reflection, my claim is that a database connection is something of a special thing. It clutters the interface when it is passed around on every call and at a certain layer of the architecture is a sufficiently cross cutting concern to be granted special treatment.

One alternative to reduce this boiler-plate is using macros with dynamic vars but that seems pretty horrible and I’m sure you’re aware of Mr Sierra’s analysis http://stuartsierra.com/2013/03/29/perils-of-dynamic-scope

With libraries like component and mount we can isolate the spread of such state to specific namespaces. There is no magic formula and its easy to change for testing purposes. We agree that mount is better and I would like to persist in its usage since I feel like it’s a better representation of a more industrial strength program.

I will look at making the demo code less hacky - the getconn method really has no business seeding the DB and the mount examples show a better way of doing this. 

On 3 January 2016 at 17:12:48, Marcelo Nomoto (notifications@github.com) wrote:

While I like mount and think it's better than component for what it does, I don't like the examples on the repo because they rely on global state when they don't need to. You can test the functions if you pass the conn as an argument without need to tinkering with the global state. You can create a datomic db and just use it on tests, they are easier to reason about. The argument were better made on http://blog.jenkster.com/2015/12/what-is-functional-programming.html Also, that's my opinion, and Micha says that there is no such thing as the right way so if you think your way is better after thinking about it, go for it ;) And feel free to open PRs if you want comments, but not everything needs it.

— Reply to this email directly or view it on GitHub.

mynomoto commented 8 years ago

@tolitius Thanks for mount, it's a great library! The issue imo is the one you pointed about argument vs toplevel db reference. But I don't think it's wrong, I just think it's better to be explicit unless there is a good reason not to. @raymcdermott Definitively this way is better than dynamic vars. Go for it then, people will have more diverse examples of code this way.

raymcdermott commented 8 years ago

OK, I have finally had time to finish up the changes. Can you please merge?