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

[#51] Ability to print message with any rgb color #53

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dariodsa
Copy link

@dariodsa dariodsa commented Apr 3, 2021

Files changed

  • colourista.cabal
  • src/Colourista/IO.hs
  • src/Colourista/Pure.hs

New behaviour

Added ability to print message using any colour which can be expressed using classic RGB representation (HEX values). If user provides a hex value with a length lower than 6, that value will be truncated with leading zeroes. So ff will produce a blue colour.

Test

*Colourista> rgbMessage "123456" "Strange colour"
Strange colour
*Colourista> rgbMessage "čć123456" "Strange color"
  🛑 Invalid hex value - čć123456
*Colourista> rgbMessage "ff" "Blue colour"
Blue colour

If the PR is going to be accepted, afterwards I can update README.md to point the users to the new feature.

@vrom911 vrom911 added the enhancement New feature or request label Apr 5, 2021
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Hey @dariodsa ,

Thanks a lot for your contribution!

I have a few comments/ideas I would like to discuss about this solution.

First of all, I see that the rgbMessage takes a String. I would propose to take 3 Word8 arguments instead and leave all the colour handling to the sRGB24 function of the package.

Also, Haskell supports Hex numbers on a lexical level, so no need to implement a parser here, it should be possible to write 0x.... 🙂

Also, I don't think that the incorrect colour should lead to error. We need to think better, what to do in this case. Maybe just warning and uncoloured text? 🤔

@dariodsa
Copy link
Author

dariodsa commented Apr 7, 2021

Thank you @vrom911 for your comments. Indeed, code now looks much more concise. 😄
Currently, I don't know to handle or even catch "overflow" errors using Word8. Currently, Haskell is kind enough to return a warning when some tries to send literal which is not in the given range.

<interactive>:34:12: warning: [-Woverflowed-literals]
    Literal 2293760 is out of the GHC.Word.Word8 range 0..255

So, now when someone tries to run a function with too high or too low literals, Haskell simply cuts its value and continue using it like it is Word8. To sum up, I think that this new error handling is much more acceptable to the end-user because the message is going to be written including the warning errors.

*Colourista> rgbMessage 0x00 0x00 0xff "I am blue."
I am blue.
*Colourista> rgbMessage 0x00 0xff 0x00 "Forest, grass, grasshopper."
Forest, grass, grasshopper.
*Colourista> rgbMessage 0xfda 0xff 0xdaa "Too much, I can't handle it."

<interactive>:39:12: warning: [-Woverflowed-literals]
    Literal 4058 is out of the GHC.Word.Word8 range 0..255

<interactive>:39:23: warning: [-Woverflowed-literals]
    Literal 3498 is out of the GHC.Word.Word8 range 0..255
Too much, I can't handle it.

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Hey @dariodsa, the implementation looks good 👍🏻 I left a few comments with some remaining improvements. And I think that relying on GHC warnings (overflow literals specifically) is a good way to go.

Ideally, for the best UI, we would like to have a compile-time compilation and allow literals only of the form 0xABCDEF but there's no easy way to achieve this in Haskell without requiring some heavyweight machinery. So API like rgbMessage 0x00 0x00 0xff looks good for me 👍🏻

src/Colourista/IO.hs Outdated Show resolved Hide resolved
src/Colourista/Pure.hs Show resolved Hide resolved
@chshersh
Copy link
Contributor

chshersh commented May 1, 2021

Hey, @dariodsa. I see there are still some conflicts in this branch with the main branch. I believe, you need to resolve them, before this branch can be merged. You can do this by either rebasing on top of main and resolving all conflicts or merging the main branch to this one. I prefer rebasing, but feel free to choose whatever method you find more convenient 🙂

@chshersh
Copy link
Contributor

Hey @dariodsa! The changes in this PR look great 👍🏻 Thanks for resolving conflicts! I see, that the code mentions the Colourista.Mode import and the HasColourMode constraint in some functions. Since those functions and modules are no longer exist, they can be removed.

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

Successfully merging this pull request may close these issues.

3 participants