elm-community / parser-combinators

A parser combinator library for Elm.
http://package.elm-lang.org/packages/elm-community/parser-combinators/latest
BSD 3-Clause "New" or "Revised" License
104 stars 13 forks source link

Make Parser type opaque? #3

Closed jvoigtlaender closed 8 years ago

jvoigtlaender commented 8 years ago

I was surprised to see that the Parser and RecursiveParser constructors are exposed to clients of the library. Is there a motivation for this?

jasonzoladz commented 8 years ago

It's useful to build new parser primitives, no? Regardless, what's the harm in having them exposed?

Bogdanp commented 8 years ago

@jasonzoladz is correct. They're exposed so that users can build their own primitive parsers should the need arise.

jvoigtlaender commented 8 years ago

I was really just surprised by the lack of encapsulation/hiding of internals. Usually in combinator libraries like this, the internals are protected, so that the users really get the impression of a domain specific language, and cannot violate any invariants. In your case now, I can violate invariants. For example, I can make a Parser value that does not correspond to a valid parser in any sense. (I can put a ParseFn inside that does not have the property that the resulting Context is a suffix of the input Context.)

I've never met a parser combinator library that exposed the internal representation. And I've met a few. But hey, it's your library. :smile:

There's also http://package.elm-lang.org/help/design-guidelines#keep-tags-and-record-constructors-secret, but Evan goes only into very little detail there about why hiding constructors is almost always the correct thing to do in library design.

jasonzoladz commented 8 years ago

I think if you want to spend time to create a pull request that makes it as fully featured as, say, Attoparsec, then @Bogdanp might take that seriously. As it stands, however, it's useful to be able to create your own primitives.

Though I suspect that your real concern is with it appearing complex to people new to Elm.

Bogdanp commented 8 years ago

In your case now, I can violate invariants.

As I've previously stated: this is by construction. The library intentionally does not hide its internals in order to give its users more flexibility. It's true that writing your own primitives can lead to breakage but I don't feel the need to protect users from themselves.

I've never met a parser combinator library that exposed the internal representation. And I've met a few. But hey, it's your library. :smile:

I fail to see how this is relevant. I'm sure many people (including myself) have used many parser combinator libraries.

There's also http://package.elm-lang.org/help/design-guidelines#keep-tags-and-record-constructors-secret, but Evan goes only into very little detail there about why hiding constructors is almost always the correct thing to do in library design.

I have read those guidelines and agree with most of them. However, in this case, I believe that exposing the internals is the correct thing to do.

A few final notes:

jvoigtlaender commented 8 years ago

@jasonzoladz, my real concern is what I said: It goes against what I know about library design (specifically for combinator libraries in typed FP languages).

As far as the motivation is to allow users to create their own primitives, my suggestion would be: Try hiding the internal representation of Parser and ParseFn, but offer a function primitive : (Context -> (Result res, Context)) -> Parser res.

And as a general strategy I would suggest: Rather than assuming that users will not be able to be content with what the library already offers in terms of primitives and combinators, take the "risk". Hide the internals. Maybe there will be no need for users to actually build new primitives. If there is, let them open issues or pull requests. That way, what comes up from that will benefit all users. Seems better to me than tempting users to first reach for "doing their own thing, by meddling with the internals", thus essentially voiding the point of using a combinator library approach in the first place.

jvoigtlaender commented 8 years ago

@Bogdanp, I seem to have annoyed you (just read your latest comment). I don't know how.

I certainly don't want to admonish you. I was making an observation that I thought might be helpful.

jvoigtlaender commented 8 years ago

Also, just for the record: I didn't claim that it was very relevant that I saw other combinator libraries before.

Bogdanp commented 8 years ago

@Bogdanp, I seem to have annoyed you (just read your latest comment). I don't know how.

You haven't. Conversational tone doesn't translate very well through writing (at least mine doesn't).

And as a general strategy I would suggest: Rather than assuming that users will not be able to be content with what the library already offers in terms of primitives and combinators, take the "risk". Hide the internals. Maybe there will be no need for users to actually build new primitives.

The motivating factor behind the creation of this library was the fact that the other library I mentioned proved to not be sufficient for writing anything except toy parsers exactly because it hid its internals. Oftentimes you can get very far with a restricted set of functionality before you realize you need more power. I believe that, in this case, providing users with an escape hatch while the library is in its early stages is the only proper thing to do.

As far as the motivation is to allow users to create their own primitives, my suggestion would be: Try hiding the internal representation of Parser and ParseFn, but offer a function primitive : (Context -> (Result res, Context)) -> Parser res.

I had considered that exact solution but even if I were to hide ParseFn and expose primitive, primitive's signature would likely have to change if ParseFn ever did so we'd be back to square one.

jvoigtlaender commented 8 years ago

I'm not going to try to convince you of anything, since you have made very conscious decisions. Even so, replies on some points:

jasonzoladz commented 8 years ago

@jvoigtlaender -- re: your bullets . . .

  1. And yet typed FP languages -- even Haskell and Idris -- all allow users freedom (IORef, MVar, FFI, unsafePerformIO). Everything is ultimately a choice.
  2. This makes no sense. Tons of open source libs expose internals and yet have contributors.
  3. It's just a parsing library. "drastic" is a funny word to use.

"I don't . . . need to protect users from themselves" really sums up what we are disagreeing about here.

jvoigtlaender commented 8 years ago
  1. Yes, it's a choice here as well. I was expressing what outcome of that choice I would have expected and for what reasons. I didn't say that anybody has to meet that expectation.
  2. I don't think I claimed anything as total as what you negate. Maybe I should have written "some users" instead of "users" in that bullet point. That's certainly what I meant, not "all users".
  3. Replace "potentially drastic" by "large" in that bullet point. The point still stands.

I don't think that the "I don't ... need to protect users from themselves" part sums it all up. I brought up other aspects I consider relevant as well.

Bogdanp commented 8 years ago

FYI I ended up going the primitive route.