@mislav Yes, it is possible to check the terminal width and take that into account when setting the wrap limit. I've added a (very hacky) commit that does that, but I'm sure this could be massively improved by someone who knows what they are doing (I've no experience with golang, I'm just making stuff work, like a complete novice).

For now, I've left the default limit at 120, as this seems as reasonable default to make text readable (it is the default in Glow too). It works reasonably well, but not perfect for all blockquoted text in issue comments etc. as these are hard-wrapped already when received from the GH API and I'm not sure what that wrap limit is (looks like it might be 140).

btw, most of that code is copied from Glow, see https://github.com/charmbracelet/glow/blob/e0789536674a25eeced8a12bd2fa17a1b6ef01a1/main.go#L156-L173

I think ideally it would use TerminalWidth() from the iostreams package in gh-cli, but I struggled to get that working.

I can take a stab at making it work.

FYI test failures are due to string equality assertions on output that was previously wrapped at 80, now 120. This can easily be fixed by just updating the assertions, but a better fix might be to allow the wrap width to be set to 80 for tests.

If the iostreams package was used to determine terminal width when setting default wrap limit, then this might be done by updating the stubbed ttySize function for tests to return 80, 24, nil rather than -1, -1, err. Alternatively, maybe an env var or config setting would make sense (this is something I considered initially, but I started simple, by hard-coding a reasonable default of 120).

I poked at this some. I think the best approach is to:

• add a RenderMarkdown to IOStreams which handles checking IOStreams.TerminalWidth
• refactor codebase to remove use of markdown.WithIO in favor of IOStreams.RenderMarkdown

What do you think, @samcoe ?

@vilmibm I think that there are only 9 call sites to markdown.Render in the codebase, 2 of which are already specifying WithWrap, and I think I would rather be explicit and just add WithWrap(io.TerminalWidth()) to these 7 call sites to make it less magic and more explicit. The downside here is we have to remember to properly set WithWrap next time we add a new place to render markdown content... although generally that code is just copy-pasted so it seems unlikely we would forget it.

@samcoe While that will fix the rendering issues associated hard-wrapping (for people with terminal cols wide enough, presumably most people), and would be a welcome change, for many people it will also result in very long lines of text that are hard to read (e.g. tput cols for me today is 364).

I think the suggestion of taking the terminal width into account, but having some maximum to prevent overly long lines would be preferred. What about adding a WithAutoWrap() function to markdown.go with IOStreams passed as an argument and used to determine the terminal width, returning a glamour.TermRendererOption representing the wrap width, set to the terminal width if less than the maximum, otherwise the maximum?

Oh, actually, maybe a better idea would be to make the wrap width a function of io, so you could call WithWrap(io.WrapWidth()), and then have io.WrapWidth() return terminal width or max width.

@mmrwoods I think the best/simplest solution here would be for WithWrap to have a max value to prevent overly long lines. The other solutions you proposed would also work, but I think keeping the markdown and iostreams package as separate as possible is the right approach. One day I hope to refactor out WithIO as well.

@samcoe makes sense, and solves both problems 👍