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

Added tokensEither #2

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mitchellwrosen
Copy link
Contributor

mitchellwrosen commented May 28, 2015

Not sure if you're interested in this function, but it's here if you want it :)

The implementation is a bit yucky - I tried to reuse the current implementation, and just sprinkled some MonadError calls throughout, then made a bogus Identity newtype with a 'throwError' that just calls 'throw'.

@feuerbach

This comment has been minimized.

Copy link
Owner

feuerbach commented May 28, 2015

I'm afraid I agree with your assessment of the implementation :-)

I'd rather you implemented this with try and unsafePerformIO. (There's also the spoon package, but it's not worth an extra dependency.)

@mitchellwrosen

This comment has been minimized.

Copy link
Contributor

mitchellwrosen commented May 28, 2015

:) How about now? - Unfortunately I had to move the orphan NFData instance into Language.Lexer.Applicative. Maybe I should add the instance in the source package and submit a PR there?

@feuerbach

This comment has been minimized.

Copy link
Owner

feuerbach commented May 28, 2015

  1. Please record a separate clean commit instead of undoing the previous one. I.e. git rebase -i, then git push -f.
  2. We actually only need to force the spine of the list. This solves all the problems associated with deepseq (orphan instance, extra constraint on the token type, extra dep) and is also more efficient (avoids doing extra work).
    The list spine can be forced by evaluating foldr (const id) () list.
@feuerbach

This comment has been minimized.

Copy link
Owner

feuerbach commented May 28, 2015

FYI, I just wrote an article with some more detail about forcing lists: https://ro-che.info/articles/2015-05-28-force-list

@mitchellwrosen

This comment has been minimized.

Copy link
Contributor

mitchellwrosen commented May 28, 2015

Neat! I think you mean forceElements though, no? We have to seq the L
EDIT: Nevermind, it's the (L ... : L ... : undefined) case :)

@mitchellwrosen mitchellwrosen force-pushed the mitchellwrosen:master branch 2 times, most recently from 37b30dc to 330550c May 28, 2015

@mitchellwrosen

This comment has been minimized.

Copy link
Contributor

mitchellwrosen commented May 28, 2015

Updated.

@feuerbach

This comment has been minimized.

Copy link
Owner

feuerbach commented May 29, 2015

Much better! Still there are issues:

  1. The build failed
  2. The fromJust . fromException bit is wrong; it will fail with Maybe.fromJust: Nothing is any other exception is thrown. You should just remove that whole line, and try will take care of catching only the right exception.
  3. There are still many unrelated changes in your commit (mostly whitespace). Make sure the patch contains only the change you actually want and claim to make.
  4. Likewise with tests. You don't have to modify any existing code there. Simply add another test that would specifically test that tokensEither works. Since it is based on tokens, there's no need to do exhaustive checking — one successful and one failing example should suffice, so that we know we're catching the exception properly.
@mitchellwrosen

This comment has been minimized.

Copy link
Contributor

mitchellwrosen commented May 29, 2015

Hm, tests passed on my machine. Must be a GHC version thing. I'll get to the other issues hopefully tonight. Apologies about whitespace changes, some of that was knee-jerk reaction to un-aligned lines, and some was my editor deleting trailing whitespace on change.

@mitchellwrosen mitchellwrosen force-pushed the mitchellwrosen:master branch from 330550c to 882f281 May 29, 2015

@mitchellwrosen

This comment has been minimized.

Copy link
Contributor

mitchellwrosen commented May 29, 2015

Updated. It wasn't a GHC version issue, I'm just an idiot and pushed broken code.

@feuerbach

This comment has been minimized.

Copy link
Owner

feuerbach commented May 29, 2015

Merged, thanks.

@feuerbach feuerbach closed this May 29, 2015

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