Improving control of colorize #459
Conversation
Issue marick#458. The test was passing before, but only because the `:times 0` was not being honored by `preprequiste`. Now the test reflects reality, but we'll want to modify implementation so that we change change `:times 1` back to `0` and still have them pass.
The environment variable MIDJE_COLORIZE is now read once, and put into the config. From there on out, it is treated just like any of the other configuration flags. Normalization of the :colorize config key was moved from the midje.emission.colorize namespace to the midje.config namespace. Previous order of precedence was (increasing order): - Config file - MIDJE_COLORIZE Previous order of precedence was (increasing order): - Config file - MIDJE_COLORIZE - Any programmatic changes to the config (e.g. via with-augmented-config)
There was a problem hiding this comment.
Thanks for contributing!
I checked out your patch and ran
export MIDJE_COLORIZE=false
lein midje :autotest
which resulted in
======================================================================
Because failures in the initial load break autotest's dependency tracking,
autotest has been cancelled.
Wrong number of args (2) passed to: colorize/note
Do you also get this error?
src/midje/emission/colorize.clj
Outdated
| (if (keyword? choice) | ||
| (name choice) | ||
| (str choice)))) | ||
| (defmacro ^:private def-colorize |
There was a problem hiding this comment.
I personally don't like using macros when functions also suffice.
In this case it could be something like
(defn- build-colorizer [normal-color reverse-color]
(fn [a-str]
(let [colorize-fn (case (config-choice :colorize)
:true normal-color
:reverse reverse-color
str)]
(colorize-fn a-str))))
(def fail (build-colorizer color/red color/red-bg))
(def pass (build-colorizer color/green color/green-bg))
(def note (build-colorizer color/cyan color/cyan-bg))There was a problem hiding this comment.
Good point, I'll refactor this.
|
@philomates I haven't seen that arity error before, but I also haven't tried the autotest functionality. I'll look into it. |
Issue marick#458. The colorizing functions should take any number of arguments (like str), but I accidentally made them only accept one.
|
@philomates I was able to reproduce the issue and fix it. |
|
I've released this under Midje |
Fixes #458.
While the internal representation of the
:colorizeconfig key has changed from a string to a keyword, this should be fully backwards compatible with whatever users have inMIDJE_COLORIZEor.midje.clj.