polysemy-research / polysemy-zoo

:monkey::panda_face: Experimental, user-contributed effects and interpreters for polysemy
BSD 3-Clause "New" or "Revised" License
70 stars 21 forks source link

Update Polysemy.Random to use the newer System.Random "Uniform" and "UniformRange" interfaces. #81

Open ShapeOfMatter opened 1 year ago

ShapeOfMatter commented 1 year ago

It's awkward using the existing Polysemy.Random, because it maps to the Random class, which the current documentation suggests shouldn't be used in new code.

This pull request fixes that, and adds relevant unit tests and QOL details.

ShapeOfMatter commented 1 year ago

Note that there are a few other ways these changes could have been handled:

tek commented 1 year ago

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

ShapeOfMatter commented 1 year ago

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

Interesting... At first I didn't like it but I'm opening to the idea. Where's the variant you use?

Anyway, I think it's a bigger change than I'm proposing in this PR. If other people are enthusiastic about it I can participate in the work, starting with opening a new Issue...

tek commented 1 year ago

It's pretty banal: https://github.com/tek/polysemy-hasql/blob/main/packages/db/lib/Polysemy/Db/Interpreter/Random.hs

anyway, I just wanted to ask your opinion, you can proceed with this without changing the effect further. I haven't used it enough to have an informed view on the matter.

so, PR looks fine to me!

ShapeOfMatter commented 1 year ago

In the variant of Random I'm using, I added the type of the random values as an effect parameter, so I'd depend on Random Int64 etc., and I can move the dependency on random to the interpreter. Any thoughts on that?

Thinking about it more, I don't think this suggestion is the way forward.

I think most people looking for a Random effect are going to want something with an off-the-shelf handler to System.Random and/or Crypto.Random (which I should add...)