Skip to content

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

Merged
paf31 merged 6 commits intopurescript-contrib:masterfrom
cdepillabout:token
Feb 21, 2016
Merged

Add Text.Parsing.Parser.Language and Token modules.#27
paf31 merged 6 commits intopurescript-contrib:masterfrom
cdepillabout:token

Conversation

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Jan 24, 2016

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

@cdepillabout
Add Text.Parsing.Parser.Language and Token modules.
4187411 
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

cdepillabout commented Jan 24, 2016

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

cdepillabout commented Jan 24, 2016

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.

michaelficarra
michaelficarra reviewed Jan 24, 2016
View reviewed changes
bower.json Outdated
Copy link
Contributor

@michaelficarra michaelficarra Jan 24, 2016

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

@cdepillabout cdepillabout Jan 24, 2016

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

cdepillabout commented Jan 26, 2016

@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

cdepillabout commented Jan 29, 2016

No worries!

paf31 added a commit that referenced this pull request Feb 21, 2016
@paf31
Merge pull request #27 from cdepillabout/token
cd65474 
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.

@jamesdbrock jamesdbrock mentioned this pull request Jan 13, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cdepillabout @paf31 @michaelficarra