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

Add atEndOfInput #27

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add atEndOfInput #27

wants to merge 1 commit into from

Conversation

k0001
Copy link
Collaborator

@k0001 k0001 commented Feb 12, 2014

I've written this function (and variants of it) a couple of times already. In fact all of pipes-aeson, pipes-binary and pipes-attoparsec include their own version of this function (see here for example https://github.com/k0001/pipes-attoparsec/blob/master/src/Pipes/Attoparsec.hs#L177)

At first I hesitated about whether it was a nice idea to include this function or not as it doesn't fit in the Parser type, but it is really useful to have it at hand, and it is compatible with the Parser type after all.

If you agree to include this, then I'll submit similar tools to pipes-bytestring and pipes-text (which ignore trailing empty Text/ByteString.

@k0001
Copy link
Collaborator Author

k0001 commented Feb 12, 2014

What I mean by saying that this is compatible with Parser is that Parser is:

forall x. StateT (Producer a m x) m r

And this function is

StateT (Producer a m r) m (Maybe r)

That is, the only difference is that the return type is constrained to be the same as the one from the underlying Producer, but other than that, one can use the same tools used with Parser when working with this function.

@Gabriella439
Copy link
Owner

Would you be okay with generalizing this to a variation on draw that returns Either r a?

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

That would be probably more useful, yes. I still won't fit in the Parser type, which might be confusing to users (though not more confusing than the proposed atEndOfInput).

What should be the name of this function? draw', drawEither?

I think that ideally I would like if returning an Either was the default behavior of draw (as it was in previous versions of pipes-parse), but then the Parser type synonym would need to change, and I'm not sure if that's a good idea. I need to think about that a bit more.

@Gabriella439
Copy link
Owner

So I was studying your code more closely and it seems that you use atEndOfParserInput you wrap it in evalStateT. Note that you can simplify this pattern by just using Pipes.next. Can you check to see if you can use next instead of atEndOfParserInput throughout your code? next is what I have been using to retrieve the final return value.

@Gabriella439
Copy link
Owner

Wait, never mind. I realized that neither next nor draw' is what you want since they both may unintentionally draw a value. Let me think about this some more. I might provide a peek equivalent of next instead.

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

I've changed it to next in pipes-binary and the result looks fine: k0001/pipes-binary@0930cb0

I understand what you say about next or draw' unintentionally drawing a value. I think that in order to check whether we are already at the end of the stream, we can just pattern match on the Pure constructor for Proxy. Maybe pipes could provide a function like the following:

done :: Proxy a' a b' b m r -> Maybe r
done (Pure r) = Just r
done _ = Nothing

The hard problem is finding a nice name for that function :)

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

Just to be clear: I'm satisfied with just using next in my code, the original proposal in this pull-request is not important anymore, you can ignore it. Still, we can continue to discuss the other matters.

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

And thanks for the hint :)

@k0001 k0001 closed this Feb 13, 2014
@Gabriella439
Copy link
Owner

So done is basically the peek function I had in mind, except the type I had in mind was this:

peek :: Monad m => Producer a m r -> m (Either r (a, Producer a m r))
peek p = do
    x <- next p
   return $ case x of
        Left   r      -> Left r
        Right (a, p') -> Right (a, yield a >> p')

The reason the result has to be wrapped in the base monad is that you have to go through M constructors, otherwise you can violate the monad transformer laws using done:

done (return r) /= done (lift (return r))

Also, since it goes through M constructors, you also have to return the new Producer, otherwise you would re-run the same monadic actions the next time you call next.

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

I agree with your general concerns about the monad transformer laws.

In your peek function I find it confusing that we are returning the a both on its own and as the first element of the returned Producer a m r. What about something like this instead?

-- | 'peek' returns 'Left r' if the given `Producer` is depleted,  
-- otherwise a 'Producer' guaranteed to be not-depleted.
peek :: Monad m => Producer a m r -> m (Either r (Producer a m r))
peek p = do
   x <- next p
   return $ case x of
        Left   r      -> Left r
        Right (a, p') -> Right (yield a >> p')

Though probably the name “peek” is not the best for this function.

@k0001
Copy link
Collaborator Author

k0001 commented Feb 13, 2014

Actually, I think your peek function is doing precisely what one would usually expect from a function named peek. Let's keep that one.

@Gabriella439
Copy link
Owner

Alright, then I'll include something like my original peek. I just need to think of a better name since peek is already taken. Here are some ideas:

  • front
  • study
  • upcoming

Feel free to suggest any other names, too. Also, I will reopen this so that I don't forget to address this.

@Gabriella439 Gabriella439 reopened this Feb 15, 2014
@PierreR
Copy link

PierreR commented Feb 15, 2014

Other naming ideas : inspect or peer.

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.

None yet

3 participants