search

polux / parsers

Parser Combinators for Dart
Other
23 stars 10 forks source link

Add Tab Stops to position calculation. #16

Closed dikmax closed 9 years ago

dikmax commented 9 years ago

Added ability to count \t as actual tab. For this I split Position to two separate classes:

Code that want not default position handling should provide function that creates position.

polux commented 9 years ago

Thank you for adding this! I was wondering if we couldn't simplify the design though: what about sticking to a single Position class like we have right now but have a new field alongside character which would be characterWithProperTabAccounting (we would have to find a better name). Then parse would just take an optional tabStop argument. Even better but maybe too heavyweight: Position could keep the current line and expose a characterWithProperTabAccounting(int tabStop) method which would compute the right position depending on the provided tabStop. Maybe that's overkill though.

dikmax commented 9 years ago

I think your ideas it too heavyweight. I think (as I see by my library) parses really need only one type of position: character or characterWithProperTabAccounting not both. And I added flexible builder function to allow arbitrary position calculation.

My second idea was to left only one TabStopPosition class but with tabStop: 1 by default. Maybe it will be much easier and compact.

polux commented 9 years ago

If you mean computationally heavyweight then one more addition is ok IMO. If you mean memory heavyweight it's just one more int.

I agree your solution is the most flexible and efficient, but it also has a more complex user interface.

I like the tabStop: 1 by default solution: it's a good compromise. It makes the user interface simpler by requiring an optional tabStop in parse instead of a PositionCreator and it makes the internal code simpler too.

dikmax commented 9 years ago

Ok, then I update code to use tabStop: 1 by default.

polux commented 9 years ago

Thanks! If you don't want to do it yourself because you consider you've done your part let me know and I'll do it. I like the "code review" approach for pull requests but I understand if you prefer to send me patches and let me deal with them. I just think it's better to reach an agreement on the API of this feature since you'll probably be its only user for now.

dikmax commented 9 years ago

Fixed.

dikmax commented 9 years ago

Ok, fixed. But I think that no one ever will use it.

polux commented 9 years ago

I think so too :) But consistency doesn't hurt. Thanks!

polux commented 9 years ago

Published version 0.14.4 to pub which contains this change. Thanks again!