fakedata-haskell / fakedata

Haskell Library for producing quality fake data
BSD 3-Clause "New" or "Revised" License
149 stars 22 forks source link

Add two more functions for company #27

Closed jappeace closed 4 years ago

jappeace commented 4 years ago

It's hard to solve the CI issues without being able to see the logs.

psibi commented 4 years ago

@jappeace Why can't you see the logs ? I can see the logs fine. Example: https://github.com/psibi/fakedata/pull/27/checks?check_run_id=1012193100

You would have to toggle the step where it's failing and see it.

jappeace commented 4 years ago

image

jappeace commented 4 years ago

Oh wait never mind, it was my ad blocker. They hosted that on a different domain for some reason.

jappeace commented 4 years ago

@psibi do you wish to match the email on a regex or something for the test? I guess I could write a regex to quickheck it.

psibi commented 4 years ago

@jappeace

@psibi do you wish to match the email on a regex or something for the test? I guess I could write a regex to quickheck it.

It's not an requirement unless you want to do it! But I think having something like that would be nice. But having a minimal test here is essential to prevent any obvious errors: https://github.com/psibi/fakedata/blob/master/test/TextSpec.hs#L399 (Note that adding that is quite easy too).

jappeace commented 4 years ago

It's not an requirement unless you want to do it!

Just leave this open, I'll add it Monday. I think we can quicheck this.

jappeace commented 4 years ago

Hi psibi, for doing quickheck I need something like Fake a ~> Gen a, Since at the moment fake uses IO as base field I'm not sure how to do that.

I want to do something like:

  prop "ends on tldr" $ forall domain $ \x -> x ~= "[a-z]+\.(com)|(nl)|(org)$"

http://hackage.haskell.org/package/QuickCheck-2.14.1/docs/Test-QuickCheck.html#v:forAll

That would generate 100 different domains from domain, and see if they match the regex. However domain is :: Fake Text, but it needs to be :: Gen Text.

psibi commented 4 years ago

@jappeace A hacky way to do that is using unsafePerfomIO. I guess it's okay, since it's for testing. Infact, hedgehog-fakedata uses that: https://github.com/parsonsmatt/hedgehog-fakedata/blob/d342c6eb5aeb9990bb36ede1d1f08becc7d71e16/src/Hedgehog/Gen/Faker.hs#L22

psibi commented 4 years ago

@jappeace I'm planning to cut a new release in a few days. Let me know if this MR is ready. Thanks!

jappeace commented 4 years ago

Hi, I shall see what I can do, I can definitely work on this next Wednesday.

jappeace commented 4 years ago

@psibi I added the tests! Please review again when you have time. Sorry for taking so long, I'm a bit overcommitted.

psibi commented 4 years ago

@jappeace It seems the tests are failing for some GHC versions. Can you see if you can fix them ? Also, I think you need to address some of the earlier comments.

Sorry for taking so long, I'm a bit overcommitted.

No hurries!

jappeace commented 4 years ago

I've been thinking, that fakeQuickcheck function may be extremely useful for the main library as well, as it makes any Fake a function compatible with QuickCheck Gen a. For riskbook alone, this means around 20 functions around custom data types suddenly become generators. Should we move it to the main library? It'll make fakedata depend on QuicCheck though, now the test suite just depends on it.

psibi commented 4 years ago

Should we move it to the main library? It'll make fakedata depend on QuicCheck though, now the test suite just depends on it.

Nah, I don't think that is proper. But I'm not opposed to you creating a separate QuickCheck package. Probably create a separate organization for fakedata packages ? We already have two:

We could also have fakedata-quickcheck there.

jappeace commented 4 years ago

We can do that as well! I guess not everyone will use fakedata with quicheck per se. There is then a small problem in that the fakedata test suite can't depend on that library because fakedata-quickcheck already depends on fakedata. How can we solve that?

psibi commented 4 years ago

@jappeace

There is then a small problem in that the fakedata test suite can't depend on that library because fakedata-quickcheck already depends on fakedata. How can we solve that?

I guess I'm not understanding it properly, but why do you think it will be a problem ? The situation as I understand is like this:

I don't see any problem with the above setup. But probably I'm missing something. :-)

jappeace commented 4 years ago

@psibi In the test suite we use this function currently, if we move it to a seperate package the test suite has to depend on that, and I think that causes a cycle (at least I got that when trying to depend on fakedata-hedgehog).

psibi commented 4 years ago

@jappeace Ah, I see what you mean. What do you think about this: We don't remove the copy of it in test suite (and there by not depend on fakedata-quickcheck). That would solve this problem:

For riskbook alone, this means around 20 functions around custom data types suddenly become generators. 

and any other fake functions too...

jappeace commented 4 years ago

Sounds good to me, the function is small anyway.

jappeace commented 4 years ago

@psibi do you have any more changes you wish from this PR?

psibi commented 4 years ago

@jappeace Thanks! Let me know if you intend to create fakedata-quickcheck, I would create an organisation and invite you there.

jappeace commented 4 years ago

@psibi I added another repo which builds here: https://github.com/jappeace/fakedata-quickcheck (It's based on the template I use to make it build because I don't understand build tools).

If you create the organisation we can move it there.

psibi commented 4 years ago

@jappeace I have invited you to the organization. Let me know if you haven't received, I will send a re-invite. Thanks!