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

Tibble vignette breaks after switching to bench #54

Closed
krlmlr opened this issue Aug 26, 2018 · 11 comments
Closed

Tibble vignette breaks after switching to bench #54

krlmlr opened this issue Aug 26, 2018 · 11 comments

Comments

@krlmlr
Copy link

krlmlr commented Aug 26, 2018

which prints tibbles that are formatted with fansi::sgr_to_html() .

Travis CI asks to contact the maintainer, "no state in first span": https://github.com/tidyverse/tibble/pull/469/files#r212818510. Could be that this is a problem in how pillar or tibble colorize their output, not sure.

To reproduce, clone the name-repair branch from tidyverse/tibble and uncomment line 46 in tibble.Rmd.

@brodieG
Copy link
Owner

brodieG commented Aug 26, 2018

This is probably a fansi issue. I'll look at it in the next couple of days.

Min reprex:

string <- c("\033[38;5;246m#&gt;\033[39m \033[38;5;246m# … with 6 more variables: \033[1mn_itr\033[22m \033[3m\033[38;5;246m&lt;int&gt;\033[38;5;246m\033[23m, \033[1mtotal_time\033[22m \033[3m\033[38;5;246m&lt;bch:tm&gt;\033[38;5;246m\033[23m,", 
"\033[38;5;246m#&gt;\033[39m #   \033[1mresult\033[22m \033[3m\033[38;5;246m&lt;list&gt;\033[38;5;246m\033[23m, \033[1mmemory\033[22m \033[3m\033[38;5;246m&lt;list&gt;\033[38;5;246m\033[23m, \033[1mtime\033[22m \033[3m\033[38;5;246m&lt;list&gt;\033[38;5;246m\033[23m, \033[1mgc\033[22m \033[3m\033[38;5;246m&lt;list&gt;\033[38;5;246m\033[23m\033[39m"
)
fansi::sgr_to_html(string)

These don't cause errors. Probably something to do with the line ending in a '38;5' color. Obviously should work anyway.

fansi::sgr_to_html(string[1])
fansi::sgr_to_html(string[2])

@brodieG
Copy link
Owner

brodieG commented Aug 26, 2018

Truly minimal:

> string <- c("\033[31m", "\033[39m")
> fansi::sgr_to_html(string)
Error in fansi::sgr_to_html(string) : 
  Internal Error: no state in first span; contact maintainer.

@krlmlr
Copy link
Author

krlmlr commented Aug 26, 2018

Thanks. The following works for me:

fansi::sgr_to_html(paste(string, collapse = "-"))

I'm using strwrap_ctl() , could this be the cause?

@brodieG
Copy link
Owner

brodieG commented Aug 27, 2018

The problem is that I implicitly assumed that styles would be terminated on each line and explicitly restarted on the next line for sgr_to_html. This was a quite silly assumption and I don't know why it ever occurred to make it in the first place. More likely, I did not realize that the code was implicitly making that assumption (shame on me).

By "I'm using strwrap_ctl", do you mean the code that produced the original problematic string from the benchmark code uses stwrap_ctl? I doubt strwrap_ctl is the issue as that one explicitly ends each element in a character vector with a terminating SGR ("\033[0m"), so it should never be the case that there is an SGR style that is intended to spill across lines as in the problematic examples here (and this is probably part of the reason I did not catch this problem earlier).

@brodieG
Copy link
Owner

brodieG commented Aug 27, 2018

Aside, your paste0(..., collapse) example is a valid work-around because now there is only one line and the bug is irrelevant since the bug affects subsequent lines when the prior line ends with an active SGR style. Since there are no prior lines, there is no problem.

@krlmlr
Copy link
Author

krlmlr commented Aug 27, 2018

Thanks. It can be a valid assumption to treat elements of a character vector as separate entities. The problem then becomes to understand how/why this input occurs.

@brodieG
Copy link
Owner

brodieG commented Aug 27, 2018

So I'm pretty sure this is happening because of tidyverse/tibble@d9c7a6d#diff-bd44a49c00a28d3d5e258721243da56aR22 where the strings are split with newlines thereby creating vector elements with active SGR at the end of the element.

This should still work, but I posted a potential workaround to the PR until I fix it (soon, hopefully). Another possible work-around that seems to work is to use fansi::strsplit_ctl instead of strsplit.

@brodieG
Copy link
Owner

brodieG commented Aug 28, 2018

So this is now fixed in the development branch. The second work-around I proposed does not actually work because of #55. That too will be fixed in the near future.

@krlmlr
Copy link
Author

krlmlr commented Aug 28, 2018

Thanks. I'm more than happy to switch to strsplit_ctl(), but then I probably should use fansi::set_knit_hooks() as shown in https://twitter.com/BrodieGaslam/status/1029059002225721344.?

@brodieG
Copy link
Owner

brodieG commented Aug 28, 2018

Note that strsplit_ctl() does unfortunately currently not work due to #55, at least for the purpose you were using it for. This the work-around I meant: https://github.com/tidyverse/tibble/pull/469/files#r213023748 (although I haven't tested thoroughly to see if that causes other problems, for example, I'm not sure why you're doing the splitting in the first place).

Re set_knit_hooks, you can certainly try that. If I have time later today I'll play around with it as this should be a great test case. However, there is no requirement that you use set_knit_hooks. set_knit_hooks just tries to automate some of what you're doing manually in the vignette, although in so doing you lose some control.

@brodieG
Copy link
Owner

brodieG commented Aug 28, 2018

FWIW, this is what it looks like with the set_hooks business, and using styles for the error output coloring:

http://htmlpreview.github.io/?https://github.com/brodieG/tibble/blob/f670703c43b4d947549370817380efd3403ebe7c/inst/doc/tibble.html

The drawback of this approach is you lose the control on how stuff is formatted. This is on my fork:

https://github.com/brodieG/tibble/tree/name-repair

@brodieG brodieG closed this as completed in 7377255 Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants