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

WIP this probably works now? #190

Closed
wants to merge 11 commits into from
Closed

WIP this probably works now? #190

wants to merge 11 commits into from

Conversation

wavebeem
Copy link
Collaborator

@wavebeem wavebeem commented Jul 4, 2017

Summary

3 core functions:

  • Parsimmon.indentMore
  • Parsimmon.indentLess
  • Parsimmon.indentSame

These consume 0 or more space characters, and update the parser state as necessary. Now all parsers take (string, index, state) instead of just (string, index), and .parse and .tryParse take an optional second parameter for the initial state, which defaults to [0], the state format expected by the Parsimmon.indent* functions.

If you check out examples/python-ish.js in this branch you should see it's fairly straightforward to parse a space-indented language with these parsers and internal changes. But it's not extensible at all, so if your indentation is more complicated then you'll have to hook directly into state tracking yourself.

Concerns

State currently may not be correctly piped through all the combinator functions. This would require a lot of extra testing to validate. The good part is it wasn't much work to make the test suite pass, and I ran this version of the library against three of my programming languages, including Squiggle, and it appeared to function correctly still.

JavaScript is filled with mutable data structures, which are a poor fit for state tracking, so exposing the internals is a bit dangerous. This state API will need to come with copious warnings.

Maybe?

  • Make the Parsimmon.indent* things into functions that take a parser?
  • Expose countSpaces?

TODO: Do not merge into master

  • Documentation
  • Changelog
  • Check out what "inconsistent dedent" means from Python
  • Add comments, blank lines to python-ish.js
  • Bring test coverage back to 100%
  • BREAKING CHANGE

My only concern is that state may not be correctly piped through all the combinator functions. This would require a lot of extra testing to validate. The good part is it wasn't much work to make the test suite pass, and I ran this version of the library against three of my programming languages, including Squiggle, and it appeared to function correctly still.
@coveralls
Copy link

coveralls commented Jul 4, 2017

Coverage Status

Coverage decreased (-5.4%) to 94.622% when pulling 97b27de on issues/158 into 10ac76e on topic/v2.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.4%) to 94.622% when pulling 97b27de on issues/158 into 10ac76e on topic/v2.

@wavebeem wavebeem mentioned this pull request Jul 4, 2017
@mixtur
Copy link

mixtur commented Jul 4, 2017

Well. You will have to either warn everyone about immutable nature of state or simply deepFreeze it.

Not sure if there is practical use case for explicit state other then indentation based languages, so probably mapState is not needed unless someone will come up with another use case.

@wavebeem
Copy link
Collaborator Author

wavebeem commented Jul 4, 2017

Well. You will have to either warn everyone about immutable nature of state or simply deepFreeze it.

Yeah, I think I'll just go with a warning in that case. It's an advanced API normally not needed.

Not sure if there is practical use case for explicit state other then indentation based languages, so probably mapState is not needed unless someone will come up with another use case.

That's fair. It's something that could be added later.

@coveralls
Copy link

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-5.2%) to 94.79% when pulling 32316cf on issues/158 into 10ac76e on topic/v2.

@coveralls
Copy link

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-5.2%) to 94.79% when pulling 19b71f9 on issues/158 into 10ac76e on topic/v2.

@coveralls
Copy link

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-5.2%) to 94.79% when pulling 0e08b07 on issues/158 into 10ac76e on topic/v2.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage decreased (-5.06%) to 94.941% when pulling 1608f62 on issues/158 into 10ac76e on topic/v2.

@coveralls
Copy link

coveralls commented Jul 8, 2017

Coverage Status

Coverage decreased (-6.7%) to 93.344% when pulling e248b37 on issues/158 into 10ac76e on topic/v2.

@coveralls
Copy link

coveralls commented Jul 8, 2017

Coverage Status

Coverage decreased (-6.7%) to 93.344% when pulling 44dfd6a on issues/158 into 10ac76e on topic/v2.

@wavebeem
Copy link
Collaborator Author

wavebeem commented Jul 9, 2017

I've come up with a better solution to the problem that doesn't involve introducing state tracking into Parsimmon, so I'm gonna decline this PR despite all the time I've sunk into it >_<

@wavebeem wavebeem closed this Jul 9, 2017
@wavebeem wavebeem deleted the issues/158 branch July 9, 2017 20:31
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