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.

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

Add Bounded Char instance #37

Merged
merged 1 commit into from
Aug 13, 2015
Merged

Add Bounded Char instance #37

merged 1 commit into from
Aug 13, 2015

Conversation

garyb
Copy link
Member

@garyb garyb commented Aug 13, 2015

This was previously an orphan in Data.Char, and also was wrong - the bottom and top values were reversed.

I'm not sure whether this is strictly speaking a breaking change or not, so not sure what we want to do about versioning here.

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

I implemented the values through the FFI to avoid having to move fromCharCode over also, by the way.

paf31 added a commit that referenced this pull request Aug 13, 2015
@paf31 paf31 merged commit bc9003d into master Aug 13, 2015
@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

Sorry, Just woke up, didn't mean to merge this. Brain is on auto-pilot. I've reverted it for now. PR looks good though.

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

Hah, no problem. Merging is fine anyway, the question is just about whether we want to bump to v0.2.0 for this... might be a bit painful as I've still not got a working auto-version-bump tool.

@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

I think we have to ... right? If someone already has Prelude and installs the latest strings, their code will be busted :(

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

Removing the Bounded Char instance from strings was a breaking change though, I versioned that one as such... it's more a case of it might break for people with the older strings if they get the newer prelude.

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

If the previous Bounded Char instance wasn't entirely broken (inverted top and bottom) I'd probably agree it is a breaking change, but I have to assume nobody was using it previously...

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

And one final point - the old strings does compile with the new Prelude, it's only a problem when the instance search is triggered.

@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

Ah ok. Maybe we can get away with it then? I don't think any code uses this instance yet.

But then, we've been bitten by things like this before :(

@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

Sorry, now I see. Yes, minor version bump for Prelude, major for strings makes sense.

@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

I'll revert my revert then?

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

Haha, sure :)

@paf31 paf31 deleted the bounded-char-instance branch August 13, 2015 14:06
@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

Alright done, I'll tag.

@paf31
Copy link
Contributor

paf31 commented Aug 13, 2015

So we need to bump everything which depends on strings, right? How many things is that?

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

It's not too bad, I've already started doing it:

purescript-enums
purescript-quickcheck
purescript-maps
purescript-datetime
purescript-sets
purescript-graphs

@garyb
Copy link
Member Author

garyb commented Aug 13, 2015

(enums needed the prelude update first)

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

Successfully merging this pull request may close these issues.

2 participants