Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 better color support #224
base: master
Are you sure you want to change the base?
Add better color support #224
Changes from all commits
35dd626
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to expose the additional exports from
Prettyprinter.Render.Terminal
too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments seem useful for users too, so how about turning them into Haddock comments?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Haddock comments to the types. Not really for the source code’s sake, but Haddock with un-haddocked definitions looks like a bit barren.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I meant for these to be Haddock comments :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's weird that the haddocks on
Color
saybut this type is now named
AnsiColor
. What's a good way to resolve this? Maybe name this typeTerminalColor
?!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used 'AnsiColor' by analogy with 'AnsiStyle', feel free to change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use
TerminalColor
. My impression is that "ANSI color" usually refers to the 8 colors from the original ANSI standard.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the
-- |
before the=
render correctly? I always avoided this style for that reason, but maybe it changed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name "palette" somehow established for this type of colours?
Alternatively,
Color256
seems like a good name to me in analogy toColor16
.Regarding the references to the
ansi-terminal
docs, direct hyperlinks would be useful, e.g.And note that modules are hyperlinked when enclosed in double-quotes (
"
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color256 would be fine, too. I was just basing it off the naming in ansi-terminal (SetPaletteColor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
Color256
would be a bit better and clearer: The haddocks forColor16
also refer to a "standard palette". Also "ColorPalette" might be misunderstood to refer to the palette itself instead of a color from the palette.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for
_
vs.Nothing
here? I don’t knowNewTerm
’s types, so making this explicit makes it clear which part you’re ignoring.