unitizer icon indicating copy to clipboard operation
unitizer copied to clipboard

better management of internal options

Open brodieG opened this issue 9 years ago • 4 comments

Thinking in particular of the options that control display lines, etc. Due to the option resetting, these are no longer during test evaluation, etc, which turns out is phenomenally annoying, but most likely the right thing to do.

A simple solution would be just to revert back to using the environment variable at top level of package to store the global options, but that feels dirty.

The other option is what we have been doing, which is making sure the global object with the stored option is passed around. This unfortunately leads to some contortions and annoying extra arguments. Case in point is the unitizerItemTestsErrors show method, since that one doesn't even accept extra arguments.

brodieG avatar Jul 01 '15 01:07 brodieG

We ended up abandoning the idea of unsetting 'unitizer' options due to the extended use of those options by display functions during review, which would have required us to make sure every one of those had some mechanism for accessing those values. Particularly problematic are functions like diff_state that might actually get called by the user at the prompt, which makes it particularly difficult to provide the default values without using options.

There are other philosophical issues as well. If we unset all the options and rely on values captured from the options prior to running unitizer, we no longer capture those options so future runs of unitizer will run with a different set of options. If we don't unset them then the users could be alerted of changes (if we allow it, currently we only do diffs on non-base options so not sure these would be reported). With things such as search.path.keep, this might be useful information. With others such as the ones controlling number of lines displayed, it shouldn't really matter since test outcomes will not be affected.

brodieG avatar Aug 14 '15 17:08 brodieG

Starting to lean towards the just allowing options and recording them. We could also separately record just the unitizer options as an object, which wouldn't allow a test to modify the unitizer options. This would be better, but requires substantially more work (e.g. diff_state, or even unitizerState, that both are accessible directly to the user but need access to the stored global options). Maybe a feature request.

One important issue with the compromise is that users can change unitizer options during tests, and that unlike pre test option updates, those are not validated in any way.

brodieG avatar Aug 16 '15 23:08 brodieG

The other issue floating around is the setting of warn=1 and error=NULL. We're very much relying on this everywhere (for an example, see #108)

brodieG avatar Aug 21 '15 02:08 brodieG

Another option to deal with: getOption("crayon.enabled").

brodieG avatar Sep 05 '16 23:09 brodieG