Skip to content
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

Re-add _Result.result as an attr which triggers a deprecation warning #88

Merged
merged 2 commits into from
Sep 16, 2017

Conversation

nicoddemus
Copy link
Member

This should fix the pluggymaster environment in pytest.

Guys I was wondering if it wouldn't be better instead of get_result() to have result as a read-only property, with the same semantics as get_result()?

outcome.result is more user-friendly than outcome.get_result(). It seems we are not using one of the key benefits of properties of being able to change an attribute access into a property at some later time without breaking users.

@goodboy
Copy link
Contributor

goodboy commented Sep 12, 2017

@nicoddemus I'm fine with either tbh.
You guys has suggested avoiding it initially but I'm personally a fan of public properties 👍

@RonnyPfannschmidt
Copy link
Member

in this case i am against properties, as a property doesn't have this implication of work or failure

from my pov a property is something that should always work

the get_result method is something that can and will fail - i'd argue it needs a better name to indicate the possibility of failure better

def result(self):
"""Get the result(s) for this hook call (DEPRECATED in favor of ``get_result()``)."""
msg = 'Use get_result() which forces correct exception handling'
warnings.warn(DeprecationWarning(msg))
Copy link
Member

Choose a reason for hiding this comment

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

this needs a stacklevel set to point at the right place

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

please set the correct stacklevel

@nicoddemus
Copy link
Member Author

Fixed warning's stacktrace, thanks for noticing @RonnyPfannschmidt

@goodboy goodboy merged commit a62cff6 into pytest-dev:master Sep 16, 2017
@RonnyPfannschmidt
Copy link
Member

👍

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