Skip to content

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