search

sckott / cowsay

cowsay w/ more animals, in R
https://sckott.github.io/cowsay/
Other
308 stars 49 forks source link

If colors are non-null, check that coloring is possible #61

Closed aedobbyn closed 6 years ago

aedobbyn commented 6 years ago

I didn't put the crayon::has_color() check in cowsay:::check_color() because it checks both what_color and by_color, so would message twice if color can't be applied. Def open to other ways of doing this, though.

sckott commented 6 years ago

thanks for this @aedobbyn ! - on vacation now, will respond mid next week

aedobbyn commented 6 years ago

No rush! Enjoy!

sckott commented 6 years ago

back from vacay.

@aedobbyn Looks like there's some test fails https://travis-ci.org/sckott/cowsay/builds/421368527#L1238-L1252 any thoughts?

aedobbyn commented 6 years ago

oh my bad, I didn't check the CI stuff. I think this is because colors on the server can't be applied so it bypasses the check and sets what_color and by_color to null instead of jumping into check_color, where the error would be triggered.

so I think we can skip those two tests if crayon::has_color is false (I'll push that change) or take some similar approach

sckott commented 6 years ago

looks good, thanks!