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

issue1625, rename getfuncargvalue to getfixturevalue #1626

Merged
merged 1 commit into from Jun 24, 2016

Conversation

Projects
None yet
5 participants
@tomviner
Contributor

tomviner commented Jun 21, 2016

Fix for #1625

getfuncargvalue will be deprecated but will still be supported for the moment.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+0.0009%) to 92.206% when pulling 0c74f4b on tomviner:issue1625/rename-getfuncargvalue into 5d8d1db on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+0.0009%) to 92.206% when pulling 0c74f4b on tomviner:issue1625/rename-getfuncargvalue into 5d8d1db on pytest-dev:master.

@@ -17,11 +17,16 @@
is specified on the command line together with the ``--pyargs``
option. Thanks to `@taschini`_ for the PR (`#1597`_).
* Rename ``getfuncargvalue`` to ``getfixturevalue``. `getfuncargvalue`

This comment has been minimized.

@The-Compiler

The-Compiler Jun 21, 2016

Member

Should probably also use double-backticks with the second getfuncargvalue as well?

@@ -17,11 +17,16 @@
is specified on the command line together with the ``--pyargs``
option. Thanks to `@taschini`_ for the PR (`#1597`_).
* Rename ``getfuncargvalue`` to ``getfixturevalue``. `getfuncargvalue`
deprecated but still present. Thanks to `@RedBeardCode`_ and `@tomviner`_

This comment has been minimized.

@The-Compiler

The-Compiler Jun 21, 2016

Member

"getfuncargvalue deprecated" -> is deprecated?

This comment has been minimized.

@The-Compiler

The-Compiler Jun 21, 2016

Member

You'll also need to add a link for @RedBeardCode to the references

This comment has been minimized.

@tomviner

tomviner Jun 21, 2016

Contributor

The linting fails because we added @RedBeardCode to Changelog links in another PR.

"""
return self._get_active_fixturedef(argname).cached_result[0]
# now deprecated
getfuncargvalue = getfixturevalue

This comment has been minimized.

@The-Compiler

The-Compiler Jun 21, 2016

Member

We might want to add a deprecation warning?

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jun 21, 2016

Might make sense to add a test for the old name as well - and a note in the doc somewhere, as well as a changelog entry?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jun 21, 2016

We had some discussion about skipping this renaming, if it is supposed to be deprecated later on altogether anyway?! (because fixture factories would be the recommended way)
Because by making it more visible and mentioning it in changelog etc this rather encourages to use it.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jun 21, 2016

I think @hpk42 also mentioned it's kind of impossible to deprecate/remove it since there are valid usecases without alternatives, and I agree. Some examples:

  • pytest itself uses it so you can use getfixture() in doctests
  • pytest-bdd heavily uses it, and I'm quite sure it's legitimate use as it dynamically generates/uses fixtures (cc @olegpidsadnyi)
  • This snippet uses it to parametrize something with two different fixtures.
  • A lot of users use it
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+0.004%) to 92.234% when pulling d3a4b2a on tomviner:issue1625/rename-getfuncargvalue into 406355f on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+0.004%) to 92.234% when pulling 2e9b714 on tomviner:issue1625/rename-getfuncargvalue into 406355f on pytest-dev:master.

@@ -2723,3 +2726,5 @@ def test_1(arg1):
*def arg1*
""")
def test_getfuncargvalue_is_deprecated(request):
pytest.deprecated_call(request.getfuncargvalue, 'tmpdir')

This comment has been minimized.

@tomviner

tomviner Jun 21, 2016

Contributor

For some reason the code above passes, but this fails:

def test_getfuncargvalue_is_deprecated_with(request):
    with pytest.deprecated_call():
        request.getfuncargvalue('tmpdir')
@tomviner

This comment has been minimized.

Contributor

tomviner commented Jun 21, 2016

Fixes added:

  • Add DeprecationWarning for usage of getfixturevalue
  • DeprecationWarning is sent
  • Parametrize test_getfixturevalue by getfixturevalue and getfuncargvalue, to ensure old version still works.
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2016

Coverage Status

Coverage increased (+0.004%) to 92.234% when pulling 16838af on tomviner:issue1625/rename-getfuncargvalue into 61ede09 on pytest-dev:master.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jun 22, 2016

@tomviner this one is in a conflicted state as well. 😁

@@ -2723,108 +2726,5 @@ def test_1(arg1):
*def arg1*
""")
class TestParameterizedSubRequest:

This comment has been minimized.

@nicoddemus

nicoddemus Jun 22, 2016

Member

Why were these tests removed?

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2016

Coverage Status

Coverage decreased (-0.1%) to 92.139% when pulling 3719e0c on tomviner:issue1625/rename-getfuncargvalue into 144dc12 on pytest-dev:master.

@tomviner

This comment has been minimized.

Contributor

tomviner commented Jun 22, 2016

@nicoddemus rebased

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jun 22, 2016

Hmmm lots of breakage now 😬, would you take a look please?

@tomviner

This comment has been minimized.

Contributor

tomviner commented Jun 22, 2016

@nicoddemus this PR was still using the old method: getfuncargvalue, and since that makes it one level deeper in the call stack, the frame traversal broke.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2016

Coverage Status

Coverage increased (+0.004%) to 92.242% when pulling 1f059c0 on tomviner:issue1625/rename-getfuncargvalue into 24179dc on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2016

Coverage Status

Coverage increased (+0.004%) to 92.242% when pulling 1f059c0 on tomviner:issue1625/rename-getfuncargvalue into 24179dc on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2016

Coverage Status

Coverage increased (+0.004%) to 92.243% when pulling 80e15b8 on tomviner:issue1625/rename-getfuncargvalue into fb79fa7 on pytest-dev:master.

@@ -200,7 +205,7 @@
Thanks `@biern`_ for the PR.
* Fix `traceback style docs`_ to describe all of the available options
(auto/long/short/line/native/no), with ``auto`` being the default since v2.6.
(auto/long/short/line/native/no), with `auto` being the default since v2.6.

This comment has been minimized.

@The-Compiler

The-Compiler Jun 22, 2016

Member

This seems like an unintended (and I think wrong) change?

This comment has been minimized.

@tomviner

tomviner Jun 24, 2016

Contributor

@The-Compiler thanks, fixed

@coveralls

This comment has been minimized.

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+0.004%) to 92.243% when pulling df9918e on tomviner:issue1625/rename-getfuncargvalue into 6359e75 on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+0.004%) to 92.243% when pulling df9918e on tomviner:issue1625/rename-getfuncargvalue into 6359e75 on pytest-dev:master.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Jun 24, 2016

LGTM, I just think this should go to the features branch. Though I'm not sure if it matters, i.e. if we're doing another release before 3.0 after the sprint.

@nicoddemus nicoddemus merged commit 3d263c6 into pytest-dev:master Jun 24, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

seanh added a commit to hypothesis/h that referenced this pull request Apr 9, 2018

Update getfuncargvalue -> getfixturevalue
getfuncargvalue was deprecated in pytest 3.0.0:

* https://docs.pytest.org/en/latest/changelog.html?highlight=getfuncargvalue#id329
* pytest-dev/pytest#1626

The latest, which we use, is pytest 3.5.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment