`Iso'` or `Getter` for `decoded`

[jdnavarro]

I'm now wondering if it was a good idea having decoded and decodedL as Iso's. It would make sense if there was not DecodingErrors ever, but with the possibility of errors it makes decoding irreversible, although encoding would still be reversible.

Perhaps we should consider decoded as a getter like in pipes-attoparsec. Or maybe it's worth having an improper Iso' in this case. What would be the consequences of violating the isomorphisms law?

This is related to #10.

[k0001]

Danny, I'm sorry, I'm a bit busy at the moment and I can't answer off the top of my head. I'll review this during the next days. Thank you.

[PierreR]

I don't really see the problem myself. Even, in case of errors, you can still replay the stream, can't you. Wrong inputs will map errors again and right inputs will map decoded values. So it looks like the isomorphism stands. As of the laws, pipes-parse already violates them. So I am not even sure it is possible to get proper iso, is it ? I am actually quite interested in the subject because I might have a go defining an Iso for pipes-aeson.

It might be nice to ask the mailing for more feedback.

[k0001]

@PierreR the problem with defining an Iso in pipes-aeson is that, for example, maybe you originally had a ByteString stream with contents such as

{ "foo":4, "bar": 5 }

… but after parsing that content into an aeson Value and then rendering it back as a ByteString you get instead

{     "bar":    5,"foo":   4}

… which is an acceptable representation, yet it's not the same one. And the Iso can't get better than this unless we implement some kind of internal buffer, something which might not be possible/efficient, I don't know. Maybe it's worth exploring this buffer idea and perhaps providing both alternatives.

I think I'm fine with this “improper iso” approach as long as we clearly state this behavior in the documentation. I've encountered a similar scenario when parsing HTTP headers: If you want to render the HTTP headers back as a ByteString after you've parsed them, say, you probably won't rendered with the same amount of whitespace.

[k0001]

Sorry, I accidentally closed the issue by clicking the wrong button in the Github interface. It's reopened now.

[jdnavarro]

In the case of pipes-binary, we could make them reversible by including whatever it was parsed until the error, in the error itself. But then having a DecodingError might not be worth because the error could be implied from leftovers. In this case we'd be back to the initial changes @Gabriel439 did.

Another option would be to offer 2 versions of decoded, one with DecodingError and another without. In attoparsec, aeson having the error is definitely worthy but is it so much in the case of binary?

I personally don't have an opinion on this yet, I'll need more time working with it to realize what really scratches my itches. But nevertheless, it'd be nice to settle this.

I'm in no hurry for this. I'd mark this as low-priority.

[Gabriella439]

I actually think decoded should be a Lens, not an Iso.

The reverse direction of the Iso is not that useful in isolation. In other words, I believe users should never use view (from decoded).

Any time the user is tempted to write something like this:

(producer ^. decoded >-> transform) ^. from decoded

They should instead either write this:

over decoded (>-> transform) producer

... or this:

producer ^. decoded >-> transform >-> for cat encode

-- Note: I'm going to introduce a separate issue to discuss adding this pipe:
encoder = for cat encode

A Lens suffices for both of those two solutions.

Not only is the Iso not necessary, but I think it's potentially harmful. I had somebody just complain to me today that they got a really nasty type error when trying to use Iso in the reverse direction, which they lpasted here:

http://lpaste.net/99623

Narrowing the type to a Lens (and providing an encoder pipe), would probably have prevented this problem. If you don't provide the user the Iso, then they will naturally gravitate to one of the two above solutions.

In fact, I think that any Iso to a type that returns an error value or unparsed continuation should be restricted to a Lens, for the same reasons.

Restricting this to a lens also provides the bonus that you can drop the profunctors dependency.

[jdnavarro]

Sorry, for the 3 previous commit references. I didn't know github reported the references so eagerly. The valid one is 1a8fbe0.

[k0001]

I'm closing this as it has been solved already.