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

Should strain test non-strict evaluation, like accumulate does? #208

Closed
petertseng opened this issue Jul 18, 2016 · 4 comments
Closed

Should strain test non-strict evaluation, like accumulate does? #208

petertseng opened this issue Jul 18, 2016 · 4 comments

Comments

@petertseng
Copy link
Member

petertseng commented Jul 18, 2016

We can make this part of #194.

Note that the accumulate tests have a test that make sure that the accumulate function is non-strict. That was added in d238a33

Is this a goal of the track that we promote non-strict evaluation when appropriate? And if so, do we want to add a similar test to strain?

They might look something like this:

  , testCase "keep non-strict" $ do
    let ws = "yes" : error "no"
    ["yes"] @=? take 1 (keep (const True) ws)
  , testCase "discard non-strict" $ do
    let ws = "yes" : error "no"
    ["yes"] @=? take 1 (discard (const False) ws)

Would want a more descriptive message than "no", of course.

@petertseng
Copy link
Member Author

petertseng commented Jul 18, 2016

Actually, maybe I misunderstand, I can't get an implementation to fail these new tests. (Edit: got it)

The proposal rules out solutions that look like the commented solution in Accumulate example:

keep f = keep' []
  where keep' acc []     = reverse acc 
        keep' acc (x:xs) = keep' (if f x then x:acc else acc) xs

Also this solution (but I expect this solution to be rare)

keep f l = keep' [] (zip l (map f l))
  where keep' acc []              = reverse acc
        keep' acc ((_, False):xs) = keep' acc xs
        keep' acc ((x, True) :xs) = keep' (x:acc) xs

@petertseng
Copy link
Member Author

petertseng commented Jul 18, 2016

Preserved in this comment: Old versions of the proposal:

  , testCase "non-strict" $ do
    let ws = ["yes", "yup", error "nope", error "no"]
    ["yes", "yup"] @=? take 2 (keep (const True) ws) 

const True doesn't cause its argument to be evaluated. That means the example failing solutions do not actually fail the test in this form. However, if the test uses (\x -> seq x True) (or as hlint suggests (seq True)) then non-strict solutions could fail.

Another:

  , testCase "non-strict" $ do
    let ws = ["yes", "yup", error "nope", error "no"]
    ["yes", "yup"] @=? take 2 (keep (`seq` True) ws) 

Both above versions have been superseded by let ws = "yes" : error "no"

@petertseng
Copy link
Member Author

petertseng commented Jul 18, 2016

Proposal changed:

v2 proposal had this:

let ws = ["yes", "yup", error "nope", error "no"]
["yes", "yup"] @=? take 2 (keep (`seq` True) ws) 

v3 proposal has this:

let ws = "yes" : error "no"
["yes"] @=? take 1 (keep (const True) ws)

seq is no longer necessary, even more non-strictness is enforced.

v2 proposal allows a CPS solution, v3 doesn't.

Losing CPS solutions is unfortunate, but I'm not sure anyone is complaining. We can go back to v2 proposal if anyone really wants to keep CPS solutions in the running. Both v2 and v3 proposal reject the solutions in earlier comment, so no need to be concerned about that when comparing v2 and v3.

@rbasso
Copy link
Contributor

rbasso commented Jul 18, 2016

  1. In accumulate, I don't think we need that check, because it's the second exercise in the track, and it's too much to ask for a complete newbie. But it's already there, and I don't think too many people get that wrong.
  2. In strain, I think that it makes sense to enforce a lazy implementation. v3 is simple enough to allow the user to understand why it failed the test. Also, I would avoid exposing users to seq unless it's really needed.

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

No branches or pull requests

2 participants