Enabling custom color themes with the GLAMOUR_STYLE environment variable#1411
Conversation
| } | ||
|
|
||
| func RenderMarkdown(text string) (string, error) { | ||
| style := "notty" |
There was a problem hiding this comment.
With this removal, we lose the feature that, when color is disabled, no coloring styles should take place. I think that notty is important to keep unless the output is to a terminal (or color was otherwise forced)
There was a problem hiding this comment.
Agreed. Will put back.
There was a problem hiding this comment.
Once the underlying issue in Glamour is resolved, I think the new implementation I just pushed should be acceptable for notty functionality.
utils/utils.go
Outdated
|
|
||
| tr, err := glamour.NewTermRenderer( | ||
| glamour.WithStandardStyle(style), | ||
| glamour.WithEnvironmentConfig(), |
There was a problem hiding this comment.
Apart from losing notty support, the switch to WithEnvironmentConfig opens us back to charmbracelet/glamour#72 right after we fixed it in #1402.
What do you think about this logic instead:
- if color is disabled:
WithStandardStyle("notty") - if GLAMOUR_STYLE is set to a simple string (no slashes):
WithStandardStyle(os.Getenv("GLAMOUR_STYLE")) - if GLAMOUR_STYLE looks like a path (e.g. has slashes):
WithStylePath(os.Getenv("GLAMOUR_STYLE"))
There was a problem hiding this comment.
I think I'd rather fix the root problem in Glamour than pollute your code base with temporary workarounds. Lemme make a PR to fix Glamour. Looks simple enough.
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Sure, it would be great it glamour fixed this upstream!
However, I'm not sure why we would wait for the upstream fix if Glamour gives us enough APIs to work around the issue anyway.
If you're keen on waiting for the upstream fix to get merged, then let us know when that happens 🙏
| // glamour.WithBaseURL(""), // TODO: make configurable | ||
| // glamour.WithWordWrap(80), // TODO: make configurable |
There was a problem hiding this comment.
I feel that it was more clear how to exactly make BaseURL and WordWrap configurable with the old instructions rather than with the new ones. Could we go back to using more verbose syntax?
There was a problem hiding this comment.
Yeah. I went with the current syntax because it seemed cleaner with the if blocks, but I guess it would be better to do it the previous way to get the increase in functionality later down the road when the TODOs are actually implemented.
|
I'll give it a day or two to see if my Glamour PR gets merged so I don't waste effort implementing a fix here just to tear it out once the fix is merged in. |
|
@mislav Got the fix implemented and merged into Glamour. |
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Fantastic! Thank you for the feature and for the upstream fix 🎉
|
Bonjour le monde |
3 participants
The Glamour library that GitHub CLI uses to render Markdown recently added support for a
GLAMOUR_STYLEenvironment variable. This variable allows users to set a persistent color theme config.This PR utilizes the new
glamour.WithEnvironmentConfig()function (which defaults toautoifGLAMOUR_STYLEis not set) to enable custom themes in the GitHub CLI.The way this PR is currently written sets the default rendering style to
autoinstead ofnotty. If there are objections I can take a stab at a minor refactor.