Skip to content

Quote strings in Show instances #86

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 1 commit into from
Aug 6, 2017
Merged

Quote strings in Show instances #86

merged 1 commit into from
Aug 6, 2017

Conversation

Rufflewind
Copy link
Contributor

Usually in Haskell, strings are shown with quotes, so I presume PureScript follows the same convention.

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! I think the only question is whether we treat this as a breaking- or patch-level change. I'm tempted to say the latter, as there's no Read class and just in general I think it's very unlikely that people are relying on the current behaviour.

@paf31
Copy link
Contributor

paf31 commented Aug 6, 2017

Patch, I agree.

@paf31 paf31 merged commit 43d8699 into purescript:master Aug 6, 2017
@paf31
Copy link
Contributor

paf31 commented Aug 6, 2017

Thanks!

@garyb
Copy link
Member

garyb commented Aug 6, 2017

Too late now, but I would have voted for patch also 😄

@paf31
Copy link
Contributor

paf31 commented Aug 6, 2017

I did make a patch change, I bumped the version to 3.3.1.

@garyb
Copy link
Member

garyb commented Aug 6, 2017

Yeah I know, I just meant my comment was too late to have swayed it were it not unanimous.

@paf31
Copy link
Contributor

paf31 commented Aug 6, 2017

Sorry for rushing, it just seemed harmless enough in this case either way, since people shouldn't be relying on (bad) Show instances anyway really.

@garyb
Copy link
Member

garyb commented Aug 6, 2017

Haha no worries, it was supposed to be a joke... I should have stayed quiet 😉

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.

4 participants