Skip to content

Iter performer #81

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

Closed

Conversation

Tomatosoup97
Copy link

@Tomatosoup97 Tomatosoup97 commented Aug 12, 2019

Hello. I've found myself in need to have actual generators among the effectful code - or in other words, to have a standard python generator that uses effects. This library already relies on yield syntax, so it's not straighforward, and thus I wrote a peformer to do that.

I think that the code turned out to be generic enough to (maybe) be incorporated into the upstream - the need for actual generators seems like a simple and reocurring thing.

Obvious issues with the PR:

  • more docs would be helpful
  • not following all code conventions (for instance you use testtools for testing)
  • added toolz - I don't really need it, since I only use curry from there. This could be rewritten to partials to cut the dependency.

I did not bother myself yet to address these issues. Instead, at first I would like to hear your opinion and whether you would like to see something like that in the repo, and then I can polish the details. Tests are here natural usage examples.

@Tomatosoup97
Copy link
Author

@radix

@radix
Copy link
Contributor

radix commented Aug 15, 2019

Thanks for the contribution! I've started looking at it, but it's going to take me a while to absorb it.

Copy link
Contributor

@radix radix left a comment

Choose a reason for hiding this comment

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

@Tomatosoup97 Can you describe a little more in-depth what your use-case for this functionality is? I can basically understand what the code is doing, but not sure where it would be useful. That would help produce better docs for it, too.

One of the things that strikes me is that it stores state in the performer functions. The way that you'd have to set up the performers seems at-odds with the general pattern I see/prefer for writing Effect-based code -- basically, that performers are all set up in one central place, near the main function of your application (along with the call to sync_perform). If I understand this correctly, code that uses this would want the state to have a finer-grained scope than a global one.

I think a way to get around this would be to introduce a third intent: one to create some state, along with the others to append to it and read from it. The result of performing the "state creation" intent would return a fresh list, then the other intents would operate on that list. This new intent could be abstracted away from any user of the decorator that you wrote, which could take care of that for them and (I think) maintain an identical API to the current one.

Still, though, I have no idea what this would actually be useful for, so I'm not sure if this suggestion really makes sense.

"""Provides retrieval of gathered values"""
return xs

return partial(performer, xs), partial(retriever, xs)
Copy link
Contributor

Choose a reason for hiding this comment

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

since xs is in the closure anyway, you don't need to explicitly take it as a parameter to performer and retriever, and don't need to explicitly partial it to bind it to those functions.

@radix
Copy link
Contributor

radix commented Nov 24, 2019

Closing this for now. If you ever feel like reopening the discussion, just comment here and I can reopen the PR.

@radix radix closed this Nov 24, 2019
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.

2 participants