Skip to content

Show Instance for CodePoints #93

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
Dec 23, 2017

Conversation

csicar
Copy link
Contributor

@csicar csicar commented Dec 21, 2017

this is useful for creating examples for the documentation

@@ -50,6 +50,9 @@ newtype CodePoint = CodePoint Int
derive instance eqCodePoint :: Eq CodePoint
derive instance ordCodePoint :: Ord CodePoint

instance showCodePoint :: Show CodePoint where
show cp = "(CodePoint " <> show (singleton cp) <> ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

This instance should definitely exist, but I think it would be better to print the contained Int in hex format. Do you know if that's possible now, or will we need to add a dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pursuit.purescript.org/packages/purescript-integers/3.2.0/docs/Data.Int#v:toStringAs would do the trick. I think, there is an advantage of displaying the Unicode representation. Maybe a combined version?
(CodePoint 0x... - "∆"). That way you can see if you have chosen the correct number for the symbol you wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be best to just use the Int, to keep it simple, and to avoid suggesting that there's anything more to a CodePoint than just an Int. People can use singleton if they want to see the code point as a string.

@hdgarrood
Copy link
Contributor

Great, thanks! There's just a few things I'd like to be done before merging:

  • Fix the warning which is causing CI to fail
  • Add purescript-integers to bower.json to explicitly depend on it; this isn't absolutely necessary since we're already pulling it in transitively but if we do it now I think it means we're less likely to run into dependency issues later
  • Add a couple of basic tests for this Show instance.

@hdgarrood
Copy link
Contributor

Thank you!

@hdgarrood hdgarrood merged commit 66b7bbc into purescript:master Dec 23, 2017
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