Skip to content

Enhance the fold type to capture terminating folds#488

Closed
adithyaov wants to merge 30 commits intomasterfrom
fold-hf
Closed

Enhance the fold type to capture terminating folds#488
adithyaov wants to merge 30 commits intomasterfrom
fold-hf

Conversation

@adithyaov
Copy link
Copy Markdown
Member

Closes #455

@adithyaov
Copy link
Copy Markdown
Member Author

I'm yet to fix the folds in the parser modules.

@adithyaov adithyaov marked this pull request as ready for review April 15, 2020 06:56
@adithyaov adithyaov force-pushed the fold-hf branch 4 times, most recently from 67adb63 to 95ea228 Compare April 15, 2020 13:01
@adithyaov adithyaov force-pushed the fold-hf branch 2 times, most recently from 774fedc to 4dbb832 Compare April 24, 2020 11:28
@adithyaov adithyaov force-pushed the fold-hf branch 2 times, most recently from e8f7efd to c6ec02d Compare July 1, 2020 04:10
@adithyaov adithyaov force-pushed the fold-hf branch 3 times, most recently from f79f964 to 3bdcb85 Compare July 11, 2020 17:10
@adithyaov adithyaov force-pushed the fold-hf branch 2 times, most recently from 645d96a to 27c7820 Compare July 23, 2020 17:20
@harendra-kumar
Copy link
Copy Markdown
Member

close and reopen.

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

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

  • Do not store the Step constructor in the fold state.
  • Write the applicative <*> as teeWith function and define the applicative in terms of that. See the parser teeWith functions. Do not store Step in the state.

@@ -311,21 +319,21 @@ either parser = Parser step initial extract
take :: Monad m => Int -> Fold m a b -> Parser m a b
take n (Fold fstep finitial fextract) = Parser step initial extract
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is same as the take in Fold. This is just fromFold (FL.take n fld).

Instead, this can be converted into: take :: Int -> Parser m a b -> Parser m a b.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It will be inconsistent with other takeist combinators.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

takeP :: Int -> Parser m a b -> Parser m a b

@@ -404,16 +412,16 @@ takeGE cnt (Fold fstep finitial fextract) = Parser step initial extract
{-# INLINE takeWhile #-}
takeWhile :: Monad m => (a -> Bool) -> Fold m a b -> Parser m a b
takeWhile predicate (Fold fstep finitial fextract) =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the same as the takeWhile in Folds i.e. fromFold . takeWhile p. This one should now be replaced by the takeWhileP of PR #597 .

@harendra-kumar
Copy link
Copy Markdown
Member

Remember to update the changelog for the behavior change of folds.

Copy link
Copy Markdown
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

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

I did not review the split using sequence functions yet. Will review those in the next iteration. Still need to check performance results.

sum = Fold (\x a -> return $ x + a) (return 0) return
sum = Fold (\x a -> return $ Partial $ x + a) (return 0) return

-- XXX Have a terminating condition here if `a == 0`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this comment.

A potential down side is that it cannot be used where only accumulators can be used.

return $ Partial n (ManyTillR 0 fs1 l)
FL.Done fb -> return $ Done n fb
-- Keep a count of elements
FL.Done1 _ -> error "Done1 nore supported in manyTill"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix typo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we are maintaining a count we can return Done cnt fb in this case? Otherwise why are we even maintaining that count, its not being used anywhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You can check the previous commit, I initially did that, but for some reason when I looked at it again, I somehow got the idea that it was wrong. Now that I look at it again, it looks fine to me.

* The only difference from the previous implementation is that the `if`
statement encompasses `collect` rather then the other way around.
@adithyaov
Copy link
Copy Markdown
Member Author

Look at #770

@adithyaov adithyaov closed this Nov 16, 2020
@adithyaov adithyaov deleted the fold-hf branch January 11, 2021 22:06
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.

Enhancements to the Fold type

2 participants