wavebeem / bread-n-butter

Parser combinators for TypeScript and JavaScript
https://bread-n-butter.wavebeem.com/
MIT License
35 stars 6 forks source link

Switch to use independent classes to represent each state of parsing is succeed or not #3

Closed minamorl closed 4 years ago

minamorl commented 4 years ago

Hi, @wavebeem ! I've read your implementation and most of design is looking good to me. IMHO, checking parsing state with string is not bad, but class-level type checking looks more sufficient and clean for me. I mean ParseOK<T> or ParseFail can be defined as classes, not as simple data structure, and it would be better for developer because easily understandable.

wavebeem commented 4 years ago

Hi @minamorl!

Thanks for reading 😄

I used to have classes for everything, but I changed the design.

New style

if (result.type === "ParseOK") {
  // ...
}

Old style

if (result.isOK()) {
  // ...
}

Alternative Old Style

if (result instanceof bnb.ParseOK) {
  // ...
}

I thought that using a type with | and no methods was easier to understand. If it is a class instead, people will want various methods on it, I think.

I decided to only use classes for things that needed methods for readability (Parser and Context).

You can see the old design with classes here: https://github.com/wavebeem/bread-n-butter/tree/a2f1fd9d143fa3a9541bc83c627b09e8b213ad3d/src

minamorl commented 4 years ago

Thanks for quick and detailed reply. I read those old code and understand why you choose current design. In TypeScript world, that works perfectly I guess.

My concern is that people who use the ES as it is are not given the benefit of types. It might be a niche for a use case but from that point of view I prefer the old style. How do you think? :)

wavebeem commented 4 years ago

I think the new style is easier for plain JS users for a few reasons:

I like both styles, but I think plain Object style will be easier to learn than custom classes.

minamorl commented 4 years ago

Using .isOK() method is not common in JS either. And you have to look at the prototype to see the method exists.

As you mentioned, it requires looking up to the prototype and the cost developers pay will be mostly same as using plain object (or higher because of console logging issue). Thank you for sharing your thoughts on this. You can close this issue. 👍

sveyret commented 4 years ago

Hi @wavebeem, Why not defining a type guard, which could be useful for both Typescript and Javascript users?

function isParseOK<T>(result: ParseOK<T> | ParseFail): result is ParseOK<T> {
  return result.type === "ParseOK"
}

EDIT: This is actually totally useless because both types contain type which is discriminent…

As a side note, I wanted to tell you that I just successfully ported a small Parsimmon-based project to BnB… :smile:

wavebeem commented 4 years ago

@sveyret

EDIT: This is actually totally useless because both types contain type which is discriminent…

Yeah I initially thought about making one of these, but you don't need it as you found out 😄

As a side note, I wanted to tell you that I just successfully ported a small Parsimmon-based project to BnB… 😄

Oh cool! If you have any thoughts or feedback, please open an Issue and tell me 😄