search

bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Simply code around `exec!` #77

Closed crisptrutski closed 8 years ago

crisptrutski commented 8 years ago

Given @danielcompton's feedback, here's a version more to my own taste. Pushing for multi-arity just made things awkward in my opinion!

You may notice that the cleanup surfaced something embarrassing - the exec-dir parameter needs to be a File, not a string as I documented..

As for the other feedback (documentation and tests), would appreciate guidance:

  1. Where besides docstring to document options - a new section in the README? Wiki?
  2. Suggestion to test option with boot - would this mean depending on or including boot-cljs-test?

Doo is welcome to absorb boot-cljs-test, it's really just another "plugin". Created a separate project simply for discoverability - "where is boot-test for CLJS?"

Otherwise maybe a non-boot test case would be better.

bensu commented 8 years ago

Thanks for doing this!

Some thoughts:

  1. We shouldn't expose :exec-dir. As it is right now it is only usable from boot-cljs-test and I can't begin to imagine the edge cases that could arise from other users. I see it more as a hack to make boot-cljs-test work now than a feature to present. Let's not advertise it in the README.
  2. I can't see how to add a regression test for boot-cljs-test without circular dependencies. Is there any way to inject the doo version for boot-cljs-test from an example project? If it is not possible and you would like to add boot-cljs-test to doo in a new folder, I could definitely invite you to the project so that you can maintain it from here. I could then run boot-cljs-test test's for every doo change the way I do for lein-doo. The big challenges would be organizing the documentation and naming: should we go for boot-doo or lein-cljs-test?

On your last point, I think that whatever test mechanism we choose should use boot since the usual source of bugs is that lein and boot have a different set of assumptions.

crisptrutski commented 8 years ago
  1. Fair point. While in principle I see the option being vaguely useful, those use cases aren't viable when bootstrap code around most CLJS builds makes assumption about your path.
  2. The boot-cljs-test only has a "test" scope dependency on doo, but injects a fixed version of doo into the project if no version is found already. So injecting is simple, I'll take a stab at just adding a build.boot file to /examples for now :smile:

Good to know you're open to assimilation, but naming is the hard part. Maybe something cheeky like cljs.test.boot and cljs.test.lein would work?

bensu commented 8 years ago

I love how "injecting is easy" when using boot. I a build.boot file in /example would make it very easy for me to test if I'm breaking boot-cljs-test.

The problem with those names is that they would get compiled inside the cljs folder, which has all the compiler code.