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

Color messages #59

Merged
merged 59 commits into from Aug 2, 2018

Conversation

3 participants
@aedobbyn
Contributor

aedobbyn commented Jun 27, 2018

Love this package.

Not sure whether coloring messages is something you'd be interested in adding, but if so, here's a small PR. I left warnings and strings as they are, so this only applies to type="message". The default color for messages remains whatever the user's default is.

This doesn't currently take advantage of everything crayon offers, like allowing users to specify background colors or nest styles. Could be cool to incorporate that, or let users choose different colors for the who and the what.

We could also add more error handling for bad colors, though the current error is decently informative:

library(cowsay) 
say(color = "foo")
#> Error in crayon::make_style(color): Unknown style specification: foo
@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jun 27, 2018

Oh and rainbow! I was trying to think of how to offer that as an option by splitting the text into 6 chunks and giving them red, orange, yellow, etc. -- maybe there's an easy way without ncharing line by line?

@sckott

This comment has been minimized.

Owner

sckott commented Jun 27, 2018

thanks @aedobbyn !!

some comments coming

R/say.r Outdated
switch(type,
message = message(sprintf(who, what)),
message = message(cat(color(sprintf(who, what)))),

This comment has been minimized.

@sckott

sckott Jun 27, 2018

Owner

hmm, does crayon still work if we don't use cat() here? idea with message() is that a user can still suppress a message with supressMessages() - but that doesn't work if there's a cat() inside message()

This comment has been minimized.

@aedobbyn

aedobbyn Jun 27, 2018

Contributor

Actually yes it does! Good point, I'll remove the cat

README.Rmd Outdated
@@ -1,3 +1,7 @@
---

This comment has been minimized.

@sckott

sckott Jun 27, 2018

Owner

can you remove this output yaml block, can just use knitr::knit("README.Rmd")

This comment has been minimized.

@aedobbyn

aedobbyn Jun 27, 2018

Contributor

no problem -- I was having some trouble knitting it without that block but seems to be fine now if the keyboard shortcut isn't used

@sckott

This comment has been minimized.

Owner

sckott commented Jun 27, 2018

I left warnings and strings as they are

did you try yet? does it work or not work?

Could be cool to incorporate that, or let users choose different colors for the who and the what.

you mean for the quote bubble thing and they animal/thing?

We could also add more error handling for bad colors, though the current error is decently informative

right, i think that error is probably good enough

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jun 27, 2018

It does work for warnings now that the cat is gone. The warning message color is still the user's default color, which looks a little weird maybe, but I'll push it so you can check it out.

As for strings, it attaches the color encoding to the string, like

> cowsay::say("hi", by = "cow", color = "purple", type = "string")
[1] "\033[38;5;129m\n ----- \nhi \n ------ \n    \\   ^__^ \n     \\  (oo)\\ ________ \n        (__)\\         )\\ /\\ \n             ||------w|\n             ||      ||\033[39m"

which I guess could be helpful.

you mean for the quote bubble thing and they animal/thing?

Yeah, exactly. So arguments would be like what_color and a by_color. Maybe overcomplicates the package?

@sckott

This comment has been minimized.

Owner

sckott commented Jun 27, 2018

Looks good on the warning and string output.

For failure behavior, how about adding an assertion that color has to be a string? cause right now we get

say(color = 5)
#> Error in crayon::make_style(color) :
#>   is.character(style) && length(style) == 1 || is_rgb_matrix(style) &&  .... is not TRUE

which isn't especially useful.

The warning message color is still the user's default color

right. not sure how to change that. if you have any ideas go for it.

Let's try what_color and by_color params and see how it goes

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jun 28, 2018

Okay so the two color thing is a bit more complicated 😊

I know pretty much nothing about how coloring strings works this but it looks like the code at the beginning of a string applies to the rest of the string until it hits an escape sequence.

If you run say on the two-colors branch on my fork (lmk if you want me to push it here), you'll see that the top line above the what text gets the by_color like it should, and the what gets the what_color. After that, the stuff below the what doesn't get a color.

For instance,

cowsay::say("hi", by = "cow", type = "message", by_color = "blue", what_color = "pink")

That as a string is

"\033[34m\n ----- \n\033[38;5;224mhi\033[39m \n ------ \n    \\   ^__^ \n     \\  (oo)\\ ________ \n        (__)\\         )\\ /\\ \n             ||------w|\n             ||      ||\033[39m"

If you cat that ^ you'll get the same thing -- the second half of the by_color isn't applied.

What we want is the by_color to pick up after the text and color the animal/thing.

It looks like we can resume the by_color by sticking the by_color's tag right after the what, so it applies to the second part of the by.

So putting \033[34m which seems to mean "make everything after this blue until you hit an escape" after the "hi". This gives us what we want.

cat("\033[34m\n ----- \n\033[38;5;224mhi\033[39m\033[34m \n ------ \n    \\   ^__^ \n     \\  (oo)\\ ________ \n        (__)\\         )\\ /\\ \n             ||------w|\n             ||      ||\033[39m")

I'd need to look more into what crayon is doing and how all of this works to see if there's a good way of going about this. Do you know much about coloring text?

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jun 28, 2018

Oh and I like the idea of checking if color is character! I'll push that in a sec.

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jun 28, 2018

Alright it's maybe a bit hacky, but two colors should work now.

If any animals/things are added that don't follow the usual pattern we might want to go to crayon::: make_chr_style where the open and close tag assignment seems to happen.

I'm not totally sure why the string matching test is failing on Travis. Passes for me locally ¯\(ツ)

@sckott

This comment has been minimized.

Owner

sckott commented Jun 28, 2018

Complicated indeed. No, I don't know much at all about coloring text.

I"ll have a look at the two colors

@@ -9,6 +9,21 @@ test_that("say types works as expected", {
# expect string on type=string
expect_is(say(type = "string"), "character")
expect_equal(

This comment has been minimized.

@sckott

sckott Jun 28, 2018

Owner

Perhaps this instead:

expect_equal(
  length(suppressMessages(say("foo", by_color = "cyan"))) + 1,
  length(suppressWarnings(say("foo", type = "warning")))
)

so that we don't get the stuff printed out in the test run

This comment has been minimized.

@aedobbyn

aedobbyn Jun 28, 2018

Contributor

Oh for sure. This test isn't even totally necessary so I'd be fine with scrapping it if you'd prefer.

@sckott

not sure on the failing test, but a suggested fix for another test

@sckott

This comment has been minimized.

Owner

sckott commented Jun 28, 2018

i'm guessing that the various parentheses and such need to be escaped?

endless_horse <- function(what="Hello world!", endless = TRUE, wait = 0.5,
what_color = NULL, horse_color = NULL) {
if (!is.null(what_color) & !is.character(what_color)) {

This comment has been minimized.

@aedobbyn

aedobbyn Jun 28, 2018

Contributor

Could pull all this stuff out into a util function

This comment has been minimized.

@sckott

sckott Jun 28, 2018

Owner

this stuff?

This comment has been minimized.

@aedobbyn

aedobbyn Jun 28, 2018

Contributor

Yeah all the checks since they're now used in endless_horse and say. It's not a ton of repetition so probably fine to leave it as is, imo

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 21, 2018

yeah I truly have no idea why multi_color was producing warnings on Appveyor and now isn't. I tried running an older version of multicolor that was failing on Appveyor on a windows 10 vm and couldn't reproduce them.
no_warnings

maybe something changed on the Appveyor side? anyway, looks like everything's working as it should
screen shot 2018-07-21 at 7 16 45 am so I think we should be good

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 21, 2018

completely unrelatedly 😆 what do you think about allowing the hypnotoad to say other things besides the only thing it can say now?

so instead of

if (by == "hypnotoad") {
    what <- "All Glory to the HYPNO TOAD!"
}

we could do something like this

if (by == "hypnotoad" && what == "Hello world!") {
    what <- "All Glory to the HYPNO TOAD!"
}

or maybe I'm not hip enough to the memes and that's against the whole idea of the hypnotoad?

@sckott

This comment has been minimized.

Owner

sckott commented Jul 23, 2018

I've no problem with hypnotoad saying other things, though should open a new issue/PR for that. maybe should check with @richfitz since he added it

@sckott

This comment has been minimized.

Owner

sckott commented Jul 23, 2018

before we merge - any thoughts on when you'd be able to get multicolor to cran? cause we'd need it up there before we submit a new ver of this to cran :)

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 24, 2018

ah good point. the only thing that I can see that would prevent multicolor from going on cran is that it uses two internal crayon functions so we'd have to ask Gábor if he'd be willing to export those

totally, will open a new PR for the hypnotoad if @richfitz is on board

@richfitz

This comment has been minimized.

Contributor

richfitz commented Jul 24, 2018

completely unrelatedly 😆 what do you think about allowing the hypnotoad to say other things besides the only thing it can say now?

I guess in theory that could be possible - not really sure why I wired it up li-ALL GLORY TO THE HYPNOTOAD

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 24, 2018

the dankest of memes...

@sckott

This comment has been minimized.

Owner

sckott commented Jul 24, 2018

thanks @aedobbyn i think we should wait to see what gabor says

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 25, 2018

sounds good!

@aedobbyn aedobbyn referenced this pull request Jul 27, 2018

Merged

Let hypnotoad say anything #60

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Jul 31, 2018

another option @jimhester suggested was to use

crayon_style_from_r_color <- get("style_from_r_color", asNamespace("crayon"))

and

is_r_color <- get("is_r_color", asNamespace("crayon"))

this way we wouldn't need to wait for a new crayon cran release.

aedobbyn added a commit to aedobbyn/multicolor that referenced this pull request Aug 2, 2018

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Aug 2, 2018

implemented this on multicolor's dev branch, and when that version is installed cowsay passes a check with 0 notes

@sckott

This comment has been minimized.

Owner

sckott commented Aug 2, 2018

cool, sounds good about Jim's idea. Hadn't tried that before. So it doesn't throw any warnings or notes on r check?

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Aug 2, 2018

nope! 0 errors | 0 warnings | 0 notes

@sckott

This comment has been minimized.

Owner

sckott commented Aug 2, 2018

looks like there's a conflict in test-say.R file

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Aug 2, 2018

oops yeah, was from the hypnotoad test addition

@sckott

This comment has been minimized.

Owner

sckott commented Aug 2, 2018

So are we good to go then do you think?

@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Aug 2, 2018

multicolor isn't on CRAN yet, but that didn't seem to bother devtools::check(cran = TRUE) since there were no notes. so I think we're good?

@sckott

sckott approved these changes Aug 2, 2018

@sckott

This comment has been minimized.

Owner

sckott commented Aug 2, 2018

ok. before we send a new version of this to cran we will need multicolor on CRAN though

@sckott

This comment has been minimized.

Owner

sckott commented Aug 2, 2018

thanks for all your work on this! LGTM

@sckott sckott added this to the v0.7 milestone Aug 2, 2018

@sckott sckott merged commit 8faba85 into sckott:master Aug 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aedobbyn

This comment has been minimized.

Contributor

aedobbyn commented Aug 2, 2018

no thank you! I'll try and get multicolor on CRAN asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment