Skip to content

Quote strings in Show instances#86

Merged
paf31 merged 1 commit into
purescript:masterfrom
Rufflewind:master
Aug 6, 2017
Merged

Quote strings in Show instances#86
paf31 merged 1 commit into
purescript:masterfrom
Rufflewind:master

Conversation

@Rufflewind

Copy link
Copy Markdown
Contributor

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

@hdgarrood hdgarrood left a comment

Copy link
Copy Markdown
Contributor

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

paf31 commented Aug 6, 2017

Copy link
Copy Markdown
Contributor

Patch, I agree.

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

paf31 commented Aug 6, 2017

Copy link
Copy Markdown
Contributor

Thanks!

@garyb

garyb commented Aug 6, 2017

Copy link
Copy Markdown
Member

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

@paf31

paf31 commented Aug 6, 2017

Copy link
Copy Markdown
Contributor

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

@garyb

garyb commented Aug 6, 2017

Copy link
Copy Markdown
Member

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

@paf31

paf31 commented Aug 6, 2017

Copy link
Copy Markdown
Contributor

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

garyb commented Aug 6, 2017

Copy link
Copy Markdown
Member

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