Skip to content

Add unicode support. #1

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
Jan 26, 2016
Merged

Add unicode support. #1

merged 6 commits into from
Jan 26, 2016

Conversation

cdepillabout
Copy link
Member

This is the same pull request from the following link, factored out into a separate module.

purescript/purescript-strings#57

In commit f8470e5, I update the documentation to be PureScript style instead of Haskell style.

There are a couple TODOs left in the code, but they can all be completed after this is merged in and released.

@cdepillabout
Copy link
Member Author

@garyb / @paf31, would you be able to do a quick* review of this?

  • This pull request is quite big, so focusing on the Data.Char.Unicode module might be a better time investment.

@paf31
Copy link
Contributor

paf31 commented Jan 19, 2016

I'll have a look over this this week, thanks! You should run your generator script though, no? And perhaps a test file would be useful as additional documentation, with the output of pulp docs.

As maintainer of a contrib project, you obviously have final say over what goes into the releases.

@cdepillabout
Copy link
Member Author

@paf31 Thanks! It's really helpful.

The generated output is in src/Data/Char/Unicode/Internal.purs. It should be part of this PR.

I'll work on getting some tests together this week.

@cdepillabout
Copy link
Member Author

@paf31 / @garyb:

Could you enable travis for this repo?
https://travis-ci.org/profile/purescript-contrib

I'm not an admin, so it looks like I can't do it.

@cdepillabout
Copy link
Member Author

Unit tests have been added cc4e892. Documentation has been added in d0277cf.

This should be ready for release.

@garyb
Copy link
Member

garyb commented Jan 20, 2016

Travis is enabled now. You might have to push again or something to get it to build the PR though, I think?

@cdepillabout
Copy link
Member Author

I've added the digitToInt function in commit ed8e045.

@cdepillabout
Copy link
Member Author

Looks like tests are passing too.

@garyb
Copy link
Member

garyb commented Jan 21, 2016

Yep, looking good!

@cdepillabout
Copy link
Member Author

If there are no objections, I'll release this in about 24 hours.

@paf31
Copy link
Contributor

paf31 commented Jan 25, 2016

Looks good to me 👍

cdepillabout added a commit that referenced this pull request Jan 26, 2016
@cdepillabout cdepillabout merged commit 00a01f6 into master Jan 26, 2016
@cdepillabout cdepillabout deleted the unicode branch January 26, 2016 14:49
@cdepillabout
Copy link
Member Author

Alright, I've released to bower and pursuit.

@paf31 and @garyb, Thanks!

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