Allow no default test dirs in config. #257
Conversation
It is helpful when a user has a non default test dir in config to also set no default test dirs to true to prevent test file conflicts and not require the option in command line.
| @@ -58,7 +59,8 @@ | |||
| 'log_level': logging.INFO, | |||
| 'results_dir': IPA_RESULTS_PATH, | |||
| 'test_files': set(), | |||
| 'timeout': 600 | |||
| 'timeout': 600, | |||
| 'no_default_test_dirs': False | |||
There was a problem hiding this comment.
I think if we use FALSE here then the other values should be True instead of None. Having the double negation makes things cumbersome ;)
There was a problem hiding this comment.
Change to default_test_dirs or use_default_test_dirs? That seems good but it is a breaking API change.
There was a problem hiding this comment.
I think we are OK breaking the API.
There was a problem hiding this comment.
The problem with that change is this arg is a flag. If the flag is provided it's set to True but by default it's none. There would be no way to turn off default directories from command line in the positive case because a default of False at the CLI prevents the value from being set in config.
If we don't want double negative I prefer multiple changes:
- By default img-proof uses default test directories. (same)
- By default if a custom set of test directories is provided only that list is used. (change)
- If a custom list is provided and the user still wants to include the default ones then passing or setting
use_default_test_dirswould make this happen. (change)
There was a problem hiding this comment.
Wrong button at first.
I am OK with the double negative, just pointing out that it requires a bit of extra mental work. It would be nice to have consistent settings, i.e. either use True/False or None/1 for example.
There was a problem hiding this comment.
From command line perspective it's either set or not so there's no hoops to jump through. And if you set it in the config file any possible truthy value will be true and vice versa.
So the distutils method that is parsing this would accept and properly handle things like; 0, 1, true, false, T, F, True, False etc. I think that's relatively user friendly.
There was a problem hiding this comment.
Ack and will note the concern but I think this is good as is for now.
It is helpful when a user has a non default test dir in config to also set no default test dirs to true to prevent test file conflicts and not require the option in command line.