hackday: gh credits by vilmibm · Pull Request #840 · cli/cli

vilmibm · 2020-04-27T19:41:41Z

We decided to merge this into the product and it's now ready for review.

The code is Not Good but it's self-contained and introduces no new third party dependencies. There are no tests for hopefully obvious reasons.

this PR adds a new top level command, gh credits:

considerations:

it defaults to showing contributors for cli/cli but you can specify any repo as a string (eg rails/rails) and it will show contributors for that repo
it will not attempt to animate if we're outputting to a pipe/file
specifying -s makes this intelligible by a screen reader
unfortunately on windows it just prints static text; the terminal there was /very/ slow at redrawing so as to preclude animation.
it will fail in spectacular and fun ways for weird/small terminal sizes. at least, narrow widths.
the code is REALLY bad because i wrote it in a very big rush

vilmibm · 2020-04-27T19:51:02Z

~~bug: stops animating too soon if window is too short. should just be skipping lines if startx < 0~~

fixed

vilmibm · 2020-05-04T19:30:19Z

we've decided to ship this after a short todo list:

write usage/help
add flag to disable animation and print something screen reader friendly
default to static thank you on windows because of animation delay looking really unpleasant
consolidate screen clearing code since we don't need to worry about windows

this commit is the result of April's hack day. it animates a thank you for a project's contributors. it only really works on linux/mac; animation woes led me to just make it static and plain on windows.

mislav

This is fun!

mislav · 2020-05-07T15:12:41Z

command/credits.go

+	return line
+}
+
+func twinkle(starLine string) string {


Best function ✨

mislav · 2020-05-07T15:14:58Z

command/credits.go

+	lines = append(lines, logins...)
+	lines = append(lines, "( <3 press ctrl-c to quit <3 )")
+
+	termWidth, termHeight, err := terminal.GetSize(int(outFile.Fd()))


Note that GetSize might not work on cygwin, where we had to use tput cols to measure terminal width

cli/utils/table_printer.go

Lines 29 to 32 in a7242f4

if w, _, err := terminal.GetSize(int(outFile.Fd())); err == nil {

ttyWidth = w

} else if isCygwin {

tputCmd := exec.Command("tput", "cols")

when we run under cygwin wouldn't we still see a runtime of windows?

mislav · 2020-05-07T15:32:11Z

command/credits.go

+func clear() {
+	// on windows we'd do cmd := exec.Command("cmd", "/c", "cls"); unfortunately the draw speed is so
+	// slow that the animation is very jerky, flashy, and painful to look at.
+	cmd := exec.Command("clear")


Note: if the overhead of shelling out turns out to be too slow for us (with a 300ms refresh rate, that might not yet be the case), you can consider clearing not by running a command, but by sending terminal escape codes

fmt.Print("\033c") - clears the screen (for me, iTerm macOS)

fmt.Println("\x1b[%dG") - since we draw the entire screen every time, we could get away without clearing. this repositions the printing cursor to the start of the terminal

ah nice; it has looked fine for me on linux (so i didn't investigate speeding up) but this is great to know

mislav · 2020-05-07T15:32:57Z

command/credits.go

+
+  gh credits            # see a credits animation for this project
+  gh credits owner/repo # see a credits animation for owner/repo
+  gh credts -s          # display a static thank you instead of animating


mislav · 2020-05-07T15:34:16Z

command/credits.go

+
+	RootCmd.AddCommand(creditsCmd)
+
+	creditsCmd.Flags().BoolP("static", "s", false, "Print a static version of the credits")


Perhaps instead of "static" in the description, we can say "Print a non-animated version of the credits"? If a person hasn't run credits yet, they might not realize that the default is animated, so it would be puzzling as to what "static" means

mislav · 2020-05-07T15:34:44Z

command/credits.go

+
+func init() {
+	rand.Seed(time.Now().UnixNano())
+	isWindows = runtime.GOOS == "windows"


Nit: this check is cheap and as such I don't think it needs to be cached in a variable

mislav · 2020-05-07T15:35:31Z

command/credits.go

+`
+
+func init() {
+	rand.Seed(time.Now().UnixNano())


Nit: since we only need to randomize seed for credits output, can we instead move this line to credits() function (and perhaps only to the animation portion)?

This is to reduce doing things in package-level init if possible

mislav · 2020-05-07T15:38:37Z

command/credits.go

+	rand.Seed(time.Now().UnixNano())
+	isWindows = runtime.GOOS == "windows"
+
+	RootCmd.AddCommand(creditsCmd)


At first I was thinking that maybe this command belongs to under gh repo, e.g. gh repo credits [<repo>]. Since this command basically displays information about a given repository, grouping it together with other gh repo commands made sense to me.

However, since the default is showing credits for cli/cli (as opposed to using the current repo), for that reason alone I think it deserves to stay as top-level command.

that was my thinking as well (about how it's mainly for seeing cli/cli's credits)

vilmibm force-pushed the credits branch from d59e7eb to c11ac0f Compare April 27, 2020 19:56

cute command to show a thank you to contributors

1aab208

this commit is the result of April's hack day. it animates a thank you for a project's contributors. it only really works on linux/mac; animation woes led me to just make it static and plain on windows.

vilmibm force-pushed the credits branch from 3e82614 to 1aab208 Compare May 4, 2020 20:42

whitespace

f427d25

vilmibm requested review from mislav and probablycorey May 4, 2020 20:49

mislav approved these changes May 7, 2020

View reviewed changes

review feedback

b15692f

vilmibm merged commit 07dc617 into master May 8, 2020

mislav deleted the credits branch June 25, 2020 14:37

	if w, _, err := terminal.GetSize(int(outFile.Fd())); err == nil {
	ttyWidth = w
	} else if isCygwin {
	tputCmd := exec.Command("tput", "cols")


		RootCmd.AddCommand(creditsCmd)

		creditsCmd.Flags().BoolP("static", "s", false, "Print a static version of the credits")

Conversation

vilmibm commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vilmibm commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vilmibm commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vilmibm commented Apr 27, 2020 •

edited

Loading

vilmibm commented Apr 27, 2020 •

edited

Loading

vilmibm commented May 4, 2020 •

edited

Loading