Skip to content

Add Text.Parsing.Parser.Language and Token modules. #27

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

Merged
merged 6 commits into from
Feb 21, 2016

Conversation

cdepillabout
Copy link
Member

This commit adds the Text.Parsing.Parser.Language and
Text.Parsing.Parser.Token modules. This is a straight port of modules
of the same name in Haskell's parsec library.

This commit also adds the (<??>) combinator, which acts like a flipped
(<?>). It is convenient to use with monadic parsers:

foo :: Parser String Unit
foo = "some message" <??> do
    string "foo"
    bar
    pure unit

This commit adds the `Text.Parsing.Parser.Language` and
`Text.Parsing.Parser.Token` modules.  This is a straight port of modules
of the same name in Haskell's `parsec` library.

This commit also adds the `(<??>)` combinator, which acts like a flipped
`(<?>)`.  It is convenient to use with monadic parsers:

```purescript
foo :: Parser String Unit
foo = "some message" <??> do
    string "foo"
    bar
    pure unit
```
@cdepillabout
Copy link
Member Author

This commit adds a dependency on purescript-unicode.

If this dependency is too heavy, there are a few ways to get around it:

  1. Release this PR as a separate library (just like what happened with purescript-unicode).

  2. Functions from purescript-unicode are being used in the following places:

    It would be possible to reimplement these functions in the Text.Parsing.Parser.Token module with functions that just handle ASCII. The downside of this is that the parsers wouldn't be able to handle some unicode inputs.

If it is decided that the dependency on purescript-unicode is okay, then I will need to release purescript-unicode before this PR can be merged.

@cdepillabout
Copy link
Member Author

One small problem I couldn't figure out was the functions near the bottom of the Text.Parsing.Parser.Token module:

https://github.com/purescript-contrib/purescript-parsing/pull/27/files#diff-13c443ec289a466b24a254814b927fa7R675

I tried to put them in the where clause of the makeTokenParser function, but I was running into the following two problems:

  1. Depending on how I wrote the functions, sometimes the compiler was crashing with an error about the list passed to foldl1 being empty.
  2. Sometimes I was getting a compiler error about not being able to have mutually recursive functions in a where clause.

I moved the functions in question to the top level and the problems disappered.

@@ -29,7 +29,9 @@
"purescript-lists": "^0.7.0",
"purescript-maybe": "^0.3.0",
"purescript-strings": "~0.7.0",
"purescript-transformers": "^0.8.1"
"purescript-transformers": "^0.8.1",
"purescript-unicode": "git://github.com/purescript-contrib/purescript-unicode#unicode",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should publish this to bower. Never actually commit git dependencies, especially when it's not a SHA.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelficarra Thanks for taking a look at this PR.

I am planning to publish purescript-unicode to bower, but I am waiting for this PR to get reviewed.

I don't think I explained it well enough in the PR comments, but this PR shouldn't be merged until purescript-unicode is released. (Or if we decide not to use the purescript-unicode dependency, then it doesn't matter.)

@paf31
Copy link
Contributor

paf31 commented Jan 24, 2016

What @michaelficarra said, plus a few code comments from me, plus:

  • We should update the LICENSE file
  • Could you please add some tests which mention haskellStyle or javaStyle specifically?

Thanks very much for working on this!

@cdepillabout
Copy link
Member Author

@paf31, Thanks for taking the time to review the code. I have made the following changes:

  • In commit 115989c, I changed the exports in Text.Parsing.Parser.Language and ext.Parsing.Parser.Token to be explicit.
  • In commit 072ef28, I changed the documentation from Haddock-style to Markdown.
  • In commit 930e594, I added Parsec's license to the LICENSE file.
  • In commit 6c2745c, I added some simple tests for the haskellStyle and javaStyle LanguageDefs.
  • In commit cc562ab, I fixed the purescript-unicode dependency in bower.json.

Now this PR should be ready for merging. (Or at least additional review.)

@paf31
Copy link
Contributor

paf31 commented Jan 28, 2016

Sorry, I'm catching up on PRs. I'll get to this soon!

@cdepillabout
Copy link
Member Author

No worries!

paf31 added a commit that referenced this pull request Feb 21, 2016
Add Text.Parsing.Parser.Language and Token modules.
@paf31 paf31 merged commit cd65474 into purescript-contrib:master Feb 21, 2016
@paf31
Copy link
Contributor

paf31 commented Feb 21, 2016

Thanks very much! And sorry again for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants