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

Add explicit error for assigning to settings attribute of state machine #1659

Merged

Conversation

nmbrgts
Copy link
Contributor

@nmbrgts nmbrgts commented Oct 27, 2018

This PR adds an explicit error to help prevent users from mistakenly assigning settings to the settings attribute of their state machine class. Ex:

class Machine(GenericStateMachine):
     pass
Machine.settings = settings(...)  # will now raise an error

I'm getting a strange error on when I run ./build.sh check-coverage locally. I'm interested to see if this error shows up in CI.

Closes #1643.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 27, 2018

  • Needs release notes in RELEASE.rst, for a minor release because we're adding a new error
  • We only need an error if the value being assigned is a settings instance.
  • I'd name it StateMachineMeta, which fits everything onto one line and fixes the mypy complaints.
  • A shorter message would also be fine, for example:
class StateMachineMeta(type):

    def __init__(self, *args, **kwargs):
        super(StateMachineMeta, self).__init__(*args, **kwargs)

    def __setattr__(self, name, value):
        if name == 'settings' and isinstance(value, Settings):
            raise AttributeError(
                'Assigning `{cls}.settings = {value} does nothing.  Assign '
                'to {cls}.TestCase.settings, or use @{value} as a decorator '
                'on the {cls} class.'.format(cls=self.__name__, value=value)
            )
        return type.__setattr__(self, name, value)


class GenericStateMachine(
    StateMachineMeta('GenericStateMachine', (object,), {})  # type: ignore
):

(and here's how you enable syntax highlighting for code blocks)

Otherwise looks good, and having a test is great!

@@ -146,8 +146,8 @@ def __setattr__(self, name, value):


class GenericStateMachine(
GenericStateMachineMeta('GenericStateMachine',
(object,), {}) # type: ignore
GenericStateMachineMeta('GenericStateMachine', # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ I need two spaces here!

Copy link
Member

Choose a reason for hiding this comment

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

And only four at the start of the line, but Mypy doesn't care about either of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how I missed that! Thanks!

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Oct 27, 2018
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Nearly there!


def test_assigning_to_settings_attribute_on_state_machine_raises_error():
with pytest.raises(AttributeError):
class StateMachine(RuleBasedStateMachine):
Copy link
Member

Choose a reason for hiding this comment

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

Use class StateMachine(GenericStateMachine): to avoid the class-defines-no-rules error.

:obj:`hypothesis.stateful.GenericStateMachine` and
:obj:`hypothesis.stateful.RuleBasedStateMachine` now raise an explicit error
when instances of :obj:`hypothesis.settings` are assigned to the classes'
settings attribute. Error directs users to use correct assignment methods.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of saying that the error directs users to correct assignment methods, we should just do that here too. e.g.

...classes' settings attribute, which is a no-op (:issue:`1643`). Instead, users should assign to ``SomeStateMachine.TestCase.settings``, or use ``@settings(...)`` as a class decorator to handle this automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That reads a lot better than what I wrote. I think I forgot my audience a bit when I was writing these notes haha

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

LGTM - I'll merge later today once the build is finished 🎉

@nmbrgts
Copy link
Contributor Author

nmbrgts commented Oct 27, 2018

Thanks Zac! And thanks for all the help along the way!

@Zac-HD Zac-HD merged commit 5f3e8b7 into HypothesisWorks:master Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants