-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add parameterize()
as an alias for parametrize()
#3159
Conversation
I'd rather not to be honest 😉 I think the warning is good enough already, and I think it's good to enforce this to be consistent across testsuites. |
I'm -0 on this for the same reason mentioned by @The-Compiler, I think the warning is a good enough approach and enforces consistency. Also I might be wrong but I don't seem much aliases because of differences in US vs UK spellings (for example color vs colour) in other libraries. Having said all that if more people really want this I'm not opposed at all, after all it is not a big maintenance problem or anything. |
I figured you were going for consistency when I saw the warning. But like @nicoddemus said, it's not that big of a chance and introduces almost no new functionality. So, if it gains enough traction you already have something started :) 🚜 Moreover, if you want consistency in the codebase, I should let you know there is an occurrence of the |
Either there should be no alternate spellings (the s variants) or this should be accepted for consistency with the other variants. Since the other variants already exist and removing them would break things, I'd say this should be accepted. I'm pretty sure I stumbled over this myself a few times. |
Which variants you mean? The one @alanbato mentions is in an internal function, which should not break anything if renamed. |
@nicoddemus sorry, I misread the source |
i'd prefer to fix the spelling for the internal helper, perhaps even move it out of the class |
As I was the one requesting this functionality, I’d vote for its inclusion. I don’t buy the color/colour argument. Americans are used to this spelling difference. However, I have to explicitly warn everyone I teach about the parametrize spelling because most people don’t know that’s a thing. If it does not impose a performance penalty or maintenance cost penalty, then removing one barrier for new users seems worth it to me. However, I’m not going to lose any sleep over it if it gets rejected. |
@@ -743,6 +743,48 @@ def __init__(self, function, fixtureinfo, config, cls=None, module=None): | |||
self._ids = set() | |||
self._arg2fixturedefs = fixtureinfo.name2fixturedefs | |||
|
|||
def parameterize(self, argnames, argvalues, indirect=False, ids=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are to include this, I prefer if you would just create an actual alias in the class:
parameterize = parametrize
While mentioning in the docs that parameterize
is a valid alias; we don't want to have the documentation duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does seem like a more elegant solution. Should it be placed below the parametrize
definition with a comment stating it's an alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Please add a test which uses the new spelling as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble getting the test to work, so far I have this:
def test_parameterize(self, testdir):
testdir.makepyfile("""
import pytest
@pytest.mark.parameterize("foo", [1, 2, 3])
def test_foo(foo):
assert foo in [1, 2, 3]
""")
reprec = testdir.inline_run()
reprec.assertoutcome(passed=3)
And it complains about fixture 'value' not found
.
If I use parametrize
instead, it does work.
What am I doing wrong? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value
? Perhaps you mean foo
? If that's the case I think you also need to handle that name here:
Line 1054 in 3bc7ced
parametrize_func = getattr(metafunc.function, 'parametrize', None) |
Perhaps something like: getattr(metafunc.function, 'parametrize', None) or getattr(metafunc.function, 'parameterize', None)
(ugh).
Otherwise please post the full error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I meant foo
. 😅 (I was also testing on a separate file where I used value
instead)
I changed it but still got the same error, posting the full error below:
Error
==================================== ERRORS ====================================
__________________________ ERROR at setup of test_foo __________________________
file /tmp/pytest-of-alanbato/pytest-30/test_parameterize0/test_parameterize.py, line 2
@pytest.mark.parameterize("foo", [1, 2, 3])
def test_foo(foo):
E fixture 'foo' not found
> available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, monkeypatch, pytestconfig, record_xml_property, recwarn, tmpdir, tmpdir_factory
> use 'pytest --fixtures [testpath]' for help on them.
/tmp/pytest-of-alanbato/pytest-30/test_parameterize0/test_parameterize.py:2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You applied the change I mentioned in fixtures.py
right? Hmm so it must be missing somewhere else... not sure where, a quick search didn't turn up anything obvious.
But I suggest for you to not spend too much time on this until we decide for sure if we want to merge this or not. 😅
For reference, here's what happens currently:
That exception should probably be less intimidating (less verbose), but other than that, it clearly says what's wrong and what to do about it. Some elaborations on why I think this is a bad idea:
|
cc @pfctdayelise who (in #463) went for a warning instead of an alias like @bgr originally proposed 😉 |
Against this change, for the reasons @The-Compiler listed. The internal naming should be fixed to be consistent. |
Guys I think overall the reaction has been a 👎 so I'm closing this. Anyways thanks everyone for joining the discussion, if nothing else at least now people are armed with arguments to justify the spelling (and there's a thread to be pointed to). |
As per the suggestion in this tweet.
I'm not sure how to test it, though.
I'm not even sure if this behavior is desired by the pytest team.
But if someone thinks this is a good idea: worry not, for it is here.
🐍