Skip to content

Collect functools.partial objects#813

Merged
nicoddemus merged 2 commits intopytest-dev:pytest-2.7from
nicoddemus:collect-functools-partial
Jul 17, 2015
Merged

Collect functools.partial objects#813
nicoddemus merged 2 commits intopytest-dev:pytest-2.7from
nicoddemus:collect-functools-partial

Conversation

@nicoddemus
Copy link
Member

Fix for issue #811.

I feel the current implementation is hackish and might break in some corner-cases.

Perhaps it would be better to don't support tests created by functools.partial objects at all, and issue a warning during collection and add a note to the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can just import functools or "partial".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@hpk42
Copy link
Contributor

hpk42 commented Jul 13, 2015

Hum, I guess we can try to support functools.partial and besides, your PR does not look too hackish to me although it would be good if we could reduce redundancy (see my code comments).

If it proves to burdensome to support functools.partials we either need to refactor or drop the feature so we probably shouldn't documented it as supported for now.

@RonnyPfannschmidt
Copy link
Member

since we consider it "unsupported" we should probably issue a warning as well

@nicoddemus
Copy link
Member Author

@hpk42:

Hum, I guess we can try to support functools.partial and besides, your PR does not look too hackish to me although it would be good if we could reduce redundancy (see my code comments).

Done. I thought it was a little hackish because we have to consider the partial arguments explicitly when extracting the fixture names from the function, but I guess that's OK.

@RonnyPfannschmidt:

since we consider it "unsupported" we should probably issue a warning as well

If we decide to decline this PR for some reason, I agree a warning is appropriate, like we do today when a Test class has an __init__ method.

@RonnyPfannschmidt
Copy link
Member

+1

@nicoddemus nicoddemus force-pushed the collect-functools-partial branch from d690780 to bc9f7f4 Compare July 13, 2015 16:45
@nicoddemus
Copy link
Member Author

Rebased on pytest-2.7 to get the mock fix to tox.ini.

@nicoddemus nicoddemus force-pushed the collect-functools-partial branch from bc9f7f4 to a7b4ed8 Compare July 16, 2015 23:37
@nicoddemus nicoddemus merged commit a7b4ed8 into pytest-dev:pytest-2.7 Jul 17, 2015
@nicoddemus nicoddemus deleted the collect-functools-partial branch July 17, 2015 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants