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.

Sign up for GitHub

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

Jump to bottom

Uppercase/lowercase on Char, tests, and more #41

Merged
merged 2 commits into from
Aug 2, 2015
Merged

Uppercase/lowercase on Char, tests, and more #41

merged 2 commits into from
Aug 2, 2015

Conversation

LiamGoodacre
Copy link
Member

Added

  • Data.Char.toUpperChar :: Char -> Char
  • Data.Char.toLowerChar :: Char -> Char

Removed

  • Data.Char.showChar :: Show Char
    • collided with showChar defined in the prelude?

Is this okay?

docs/Data.String.md changed when I generated the docs, someone didn't regenerate them last time I guess?

@LiamGoodacre
Uppercase/lowercase on Char
ecc6ad6 
Added

* `Data.Char.toUpperChar :: Char -> Char`
* `Data.Char.toLowerChar :: Char -> Char`

Removed

* `Data.Char.showChar :: Show Char`
  - collided with `showChar` defined in the prelude?
@LiamGoodacre
Copy link
Member Author

Removing showChar seemed to be the only way I could get it to run in psci.

@garyb
Copy link
Member

garyb commented Jul 16, 2015

How about just toUpper/toLower for these?

@LiamGoodacre
Copy link
Member Author

Originally I went with toUpper and toLower, however those names are also defined in Data.String. I thought that would be a bit strange to export things with the same name from different parts of the same module.

However, I am happy to rename if you're okay with that 😃

@LiamGoodacre
tests, changes, and fixes
1523fc3 
Testing
--

* Added `test/` directory.
* Added `pulp test` to the `build` script.
* New dev dependencies `purescript-assert` and `purescript-console`

Breaking changes
--

* `Data.String.Regex.search` now gives back `Maybe Int`.
    * `Nothing` for no match instead of `-1`.
* `Data.Char.toUpperChar` is now `Data.Char.toUpper`.
* `Data.Char.toLowerChar` is now `Data.Char.toLower`.

Bug fixes
--

* `Data.String.indexOf'` wrong for index out of bounds.
* `Data.String.lastIndexOf'` wrong for index out of bounds.
* `Data.String.localeCompare` off by 1.

Notes
--

The vast majority of functions have at least one test case.

Lots of room for more though, especially in `Data.String.Regex`!

Tests could also be added for checking that the unsafe functions throw in the documented ways.
@LiamGoodacre LiamGoodacre changed the title Uppercase/lowercase on Char Uppercase/lowercase on Char, tests, and more Jul 18, 2015
@garyb garyb mentioned this pull request Jul 18, 2015
Merged
@LiamGoodacre
Copy link
Member Author

@paf31, @garyb; Does this all look okay?

@garyb
Copy link
Member

garyb commented Jul 18, 2015

Looks great to me 👍

@garyb
Copy link
Member

garyb commented Jul 22, 2015

I've not merged this yet, as doing so will potentially unleash chaos when we bump the version 😄

I have a tool that I need to finish up that will allow us to bump all the dependencies together, once that's working I'll merge/release this. And thanks!

@garyb
Copy link
Member

garyb commented Jul 22, 2015

It's not too heavily depended-on actually, but even still, I'd rather use the tool to do it:

  • purescript-enums
  • purescript-strongcheck
  • purescript-base
  • purescript-datetime
  • purescript-quickcheck
  • purescript-maps
  • purescript-argonaut-core
  • purescript-argonaut-codecs
  • purescript-argonaut-traversals
  • purescript-argonaut
  • purescript-sets
  • purescript-graphs

garyb added a commit that referenced this pull request Aug 2, 2015
@garyb
Merge pull request #41 from LiamGoodacre/master
24a912f 
Uppercase/lowercase on Char, tests, and more
@garyb garyb merged commit 24a912f into purescript:master Aug 2, 2015
@garyb
Copy link
Member

garyb commented Aug 2, 2015

Thanks for your work on this, sorry it took so long to get merged.

@LiamGoodacre LiamGoodacre mentioned this pull request Aug 2, 2015
Closed
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LiamGoodacre @garyb