-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add hyperlink template function #72
Conversation
Relates to cli/cli#6179
146f79e
to
39db6d3
Compare
pkg/template/template.go
Outdated
output io.Writer | ||
tmpl *template.Template | ||
tp tableprinter.TablePrinter | ||
width int | ||
} | ||
|
||
// New initializes a Template. | ||
func New(w io.Writer, width int, colorEnabled bool) Template { | ||
func New(w io.Writer, width int, colorEnabled, isTTY bool) Template { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should add an isTTY property to a template. So far, we've kept template output static, no matter where it is connected to. All template helpers have predictable outputs, and the only one that changes dynamically is autocolor
.
I'd say that hyperlink
should always output the hyperlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but then aren't we pushing the work to everyone who uses a template? Alternatively, what if we add an isTTY
(or something) function that template conditions can use? Or is the assumption that the caller is always in control of both the template and the output, so they can easily and always adapt as needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered that tableprinter.New
takes an isTTY
and did respond appropriately so that it was TSV otherwise. Feels like this was regressed. Perhaps we should take an isTTY
? Or was it ever further discussed to move much/all of iostreams
to go-gh? @samcoe and I had briefly mentioned it in an issue - I don't think it was even this repo.
Alternatively, if the concern is breaking changes that could grow in time, what about using the WithOptions
pattern that tableprinter
is using as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that last time we've had this discussion was in cli/cli#3519 (comment)
We've opted to always default to TTY output for consistency so that the user of --template
doesn't have to guess the actual format of the final output depending on the end-user's execution context. If the gh developer wants separate formats for TTY vs script output, yes in theory they should use two separate templates. But I think --template
should be mostly for human-readable outputs for terminals anyway.
pkg/template/template.go
Outdated
text = link | ||
} | ||
|
||
return fmt.Sprintf("\x1b]8;;%s\x1b\\%s\x1b]8;;\x1b\\", link, text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this link to the hyperlink "spec"? Thanks
@mislav I made the changes you requested, but I think it's least worth considering (perhaps not for this PR) whether more should be done in the template functions like they were in cli/cli. Now every script - every invocation - has to vary the template based on expected output. Maybe that actually ends up being rare enough since the target of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
Relates to cli/cli#6179