[RFC] config: addini: support choices#5968
[RFC] config: addini: support choices#5968blueyed wants to merge 1 commit intopytest-dev:featuresfrom
Conversation
blueyed
force-pushed
the
addini-choices
branch
from
b30e766 to
5b88f77
Compare
blueyed
added
topic: config
blueyed
changed the title
nicoddemus
left a comment
nicoddemus
left a comment
There was a problem hiding this comment.
This looks great as a start, thanks!
Besides what you already mention in your TODO: list in the description, we should add an example to the documentation too.
| raise | ||
|
|
||
| if ns.override_ini: | ||
| for ini_config in ns.override_ini: |
There was a problem hiding this comment.
Suggestion: can we please move all the validation code to a separate function, and make that function reuse the work done by _validate_ini_value to avoid duplication?
There was a problem hiding this comment.
Maybe, but I would also rather like to keep this "minimal" / "a no brainer" patch for now.
Can be refactored in the end maybe then.
There was a problem hiding this comment.
yup, please add a follow-up issue
| return optparser.parse_known_args(args, namespace=namespace) | ||
|
|
||
| def addini(self, name, help, type=None, default=None): | ||
| def addini(self, name, help, type=None, default=None, choices=None): |
There was a problem hiding this comment.
Let's take this opportunity and make choices keyword-only
| def addini(self, name, help, type=None, default=None, choices=None): | |
| def addini(self, name, help, type=None, default=None, *, choices=None): |
| :type: type of the variable, can be ``pathlist``, ``args``, ``linelist`` | ||
| or ``bool``. | ||
| :type: type of the variable, can be ``pathlist``, ``args``, ``linelist``, | ||
| ``bool``, or ``type``. |
There was a problem hiding this comment.
| ``bool``, or ``type``. | |
| ``"bool"``, or ``"choice"``. |
?
| :name: name of the ini-variable | ||
| :type: type of the variable, can be ``pathlist``, ``args``, ``linelist`` | ||
| or ``bool``. | ||
| :type: type of the variable, can be ``pathlist``, ``args``, ``linelist``, |
There was a problem hiding this comment.
| :type: type of the variable, can be ``pathlist``, ``args``, ``linelist``, | |
| :type: type of the variable, can be ``"pathlist"``, ``"args"``, ``"linelist"``, |
| """ | ||
| assert type in (None, "pathlist", "args", "linelist", "bool") | ||
| self._inidict[name] = (help, type, default) | ||
| # TODO: callable type? |
There was a problem hiding this comment.
Not sure we need to make this a callable, after all the user can always call the callable before passing the choices to this function... or do you have a specific use case in mind?
There was a problem hiding this comment.
It was meant to work like type in argparse then - which is more universal than having choices (only).
There was a problem hiding this comment.
Oh I thought you meant choices being able to be a callable, my bad.
We can add type here no problem, following the same semantics that type and choices have in argparser. I will leave this up to you if you prefer to do this in this PR or do it separately.
| self._processopt = processopt | ||
| self._usage = usage | ||
| self._inidict = {} # type: Dict[str, Tuple[str, Optional[str], Any]] | ||
| self._inidict = ( |
There was a problem hiding this comment.
I think this have grown enough to deserve a namedtuple or attrs instance. What do you think?
| """ | ||
| ) | ||
| config = testdir.parseconfig() | ||
| assert config.getini("mychoice") == "def" |
There was a problem hiding this comment.
I think the user should explicitly state "def" in the choices list.
| print('\\nmychoice:%s\\n' % ini_val)""" | ||
| ) | ||
| result = testdir.runpytest("-s", str(p1)) | ||
| result.stdout.fnmatch_lines( |
There was a problem hiding this comment.
This seems to be tested by the previous with pytest.raises already
| ) | ||
| assert result.ret == ExitCode.TESTS_FAILED | ||
|
|
||
| p1 = testdir.makepyfile( |
There was a problem hiding this comment.
I usually prefer to set the data in some module (say sys) and inspect it later:
monkeypatch.setattr(sys, "MY_INI_VAL", None, raising=False)
p1 = testdir.makepyfile(
"""
import sys
def test_pass(pytestconfig):
ini_val = pytestconfig.getini("mychoice")
sys.MY_INI_VAL = pytestconfig.getini("mychoice")
print('\\nmychoice:%s\\n' % ini_val)"""
)
result = testdir.runpytest("-s", "-o", "mychoice=one", str(p1))
assert sys.MY_INI_VAL == "one"
assert result.ret == ExitCode.OKBut it is merely a comment in case you like this approach.
3 participants
Fixes #5057.
TODO:
type(validation), could be used to providechoicesinternallydefault=None- currently returns[]then #6117 (comment)