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 API to remove items from stateful.Bundle #136

Closed
teh opened this issue Aug 28, 2015 · 8 comments
Closed

Add API to remove items from stateful.Bundle #136

teh opened this issue Aug 28, 2015 · 8 comments
Labels
enhancement it's not broken, but we want it to be better

Comments

@teh
Copy link

teh commented Aug 28, 2015

Hey, I'm testing an API that has add and remove functionality using RuleBasedStateMachine.

E.g.

class OrganisationAPI(RuleBasedStateMachine):
    orgs = Bundle('Organisation')
    users = Bundle('User')

    @rule(target=orgs, user=users, name=st.text(average_size=5))
    def create_org(self, user, name):
        return api.organisation_create(name, user)

    @rule(org=orgs, user=users)
    def delete_org(self, org, user):
        return api.organisation_delete(org, user)

Ideally the org bundle would not re-used deleted orgs. Filing as a reminder to myself, will investigate how to implement.

@DRMacIver
Copy link
Member

Yep. I definitely agree this is a good idea. It's been vaguely on my TODO list for a while - implementation wise it should be pretty straightforward (behind the scenes there's just a set of live variable names in each bundle) but I didn't have an API I really liked for this. Very happy to take suggestions.

@teh
Copy link
Author

teh commented Aug 29, 2015

I'm going to throw out a few ideas but I'm not a fan of any of them.

  1. Add @rule(remove_from=orgs, ...)
  2. self.orgs.remove(...) - but I guess this prevents the machine's way to reason about where data enters and leaves the system?
  3. When we return a value for target= we actually return a wrapper with a .remove() and a.get_value() method.

No 3 could fix a related problem I often have: Depending on the state of my model I don't always have a valid target to return. I could return NotValid() to signal hypothesis to not store the returned value.

But no 3 is also much uglier and makes everything much less nice (implementation and interface).

@DRMacIver
Copy link
Member

The API sketch I had that I was most happy with was to provide a consumes() function so you could do something like rule(source=consumes(org)).

In particular you can't just remove a value. It might be assigned to multiple variables.

One thing to consider: The API I would like to move towards for the rule based state machine allows bundles to be used as if they were strategies. so e.g. you should be able to have x=lists(orgs). It would be nice to have an API which would not get in the way when that comes around. The consumes() API does have the advantage of probably being compatible with this, though it may complicate the implementation significantly.

I have not yet figured out the details of this and supporting it is definitely not a requirement, but it's something to bear in mind as needing to not block.

RE the related issue: The way I usually deal with those is with assume(). So you can do assume(can return valid result); return (valid result)

@teh
Copy link
Author

teh commented Aug 29, 2015

To clarify: consumes(org) means that whatever org instance is passed in will be no longer in the Bundle after the function has been executed?

Thanks for assume I didn't expect it to work in the state machine context. Nice design!

@DRMacIver
Copy link
Member

More specifically it means the current name for the org instance would not be reused. In particular if you had something like

@rule(target=orgs, source=orgs)
def copy_org(self, o):
    return o

Then a single organisation would appear in the list of available orgs multiple times and only one of those would be removed from the list.

@DRMacIver DRMacIver added enhancement it's not broken, but we want it to be better help-wanted labels Sep 25, 2015
@jomuel
Copy link
Contributor

jomuel commented Dec 18, 2018

Hi @Zac-HD, I see you removed the help-wanted label on this issue but is it still open to external contributors? I would love to have this feature in Hypothesis for my work and would have time to give it a try during the next weeks.

@Zac-HD
Copy link
Member

Zac-HD commented Dec 18, 2018

Absolutely! We removed the help-wanted label because we'd love help with any issue, so it wasn't much use as a classifier 😉

My main tip for this is that we will want to make sure that the API is flexible enough to stick with long-term. Adding it to Hypothesis might be substantially more work than getting it to "useful for me", so we have a standing offer that you can hand an in-progress PR over for maintainers to finish if you're sick of it.

So go ahead, good luck, thanks, and ask for tips if you need some!

@jomuel
Copy link
Contributor

jomuel commented Dec 27, 2018

Figured out the problems with the build now, please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better
Projects
None yet
Development

No branches or pull requests

4 participants