Create gh search code command

[joshkraft]

Closes #6945

NOTE: These mockups are now outdated - see later comments for updated visuals

Light Theme

Dark Theme

[joshkraft]

I am open to suggestions on text style for this feature (color and boldness). I initially went with yellow, but it was hard to see in a light terminal. Cyan is pretty readable in both light and dark terminals.

[joshkraft]

In general, displayResults ended up being a bit more complicated than I initially expected. I am pretty new to Go, and I am open to suggestions on ways to make this simpler / more idiomatic.

A few factors at play here:

1. The code search API does not use the limit flag as you might expect. The limit seems to apply to the number of files returned, but each file might have multiple 'fragments' returned that match the query, and each 'fragment' can also have multiple matches within it. As discussed here, I used limit both when making the request and when rendering the results to stop rendering search results after limit rows had been displayed.

2. The code search API (as far as I can tell) does not provide line numbers for the matches. As such, rather than rendering line numbers for each result (as discussed here), I chose to render each line with a simple counter that tracks with the number of lines printed for each file. I can easily remove this if desired.

3. Some of the complexity of this code is because each line in the 'fragment' can have 0, 1, or many matches within it. We want to skip lines with 0 matches, and print each line with 1 or more matches just once. I have a feeling there is a more elegant solution than what I came up with.

[samcoe]

I think what you have is solid, and getHighlightIndices is pretty clever. I took a quick pass at making it a bit simpler and idiomatic, let me know what you think:

func displayResults(io *iostreams.IOStreams, results search.CodeResult, limit int) error {
	cs := io.ColorScheme()
	tp := tableprinter.New(io)
	displayed := 0
outer:
	for _, code := range results.Items {
		if io.IsStdoutTTY() {
			header := fmt.Sprintf("%s %s", cs.GreenBold(code.Repo.FullName), cs.GreenBold(code.Path))
			tp.AddField(header)
			tp.EndRow()
		}
		for _, match := range code.TextMatches {
			fragments := formatMatch(cs, match)
			for i, fragment := range fragments {
				if io.IsStdoutTTY() {
					tp.AddField(fmt.Sprintf("%s. %s", strconv.Itoa(i+1), fragment))
				} else {
					tp.AddField(fmt.Sprintf("%s %s: %s", code.Repo.FullName, code.Path, fragment))
				}
				tp.EndRow()
				displayed++
				if displayed == limit {
					break outer
				}
			}
		}
	}
	if io.IsStdoutTTY() {
		header := fmt.Sprintf("Showing %d of %d results\n\n", displayed, max(displayed, results.Total))
		fmt.Fprintf(io.Out, "\n%s", header)
	}
	return tp.Render()
}

func formatMatch(cs *iostreams.ColorScheme, tm search.TextMatch) []string {
	shouldHighlight := matchHighlightMask(tm.Fragment, tm.Matches)
	var lines []string
	var found bool
	var b strings.Builder
	for i, c := range tm.Fragment {
		if c == '\n' {
			if found {
				lines = append(lines, b.String())
			}
			found = false
			b.Reset()
			continue
		}
		if shouldHighlight[i] {
			b.WriteString(cs.CyanBold(string(c)))
			found = true
		} else {
			b.WriteRune(c)
		}
	}
	return lines
}

func matchHighlightMask(fragment string, matches []search.Match) []bool {
	m := make([]bool, len(fragment))
	for _, match := range matches {
		start := match.Indices[0]
		stop := match.Indices[1]
		for i := start; i < stop; i++ {
			m[i] = true
		}
	}
	return m
}

[joshkraft]

Nice, I haven't utilized labels very often but it is a nice fit here. I incorporated these suggestions, with one small tweak - I had to add back in a variable to keep track of the number of overall displayed matches per item in results.Items, so that we can label the results properly as we move through the multiple TextMatches possible for each item/file. I haven't found a way a good way to get rid of this variable without dropping the ability to number the matches for each file.

[joshkraft]

I started thinking about validating/sorting the intervals provided by the search API, but this seemed like a simple way to get around having to solve that problem. Open to suggestions here as well.

[samcoe]

@joshkraft This is coming along excellent! The output with the highlighting looks really elegant in my opinion. I left a handful of comments in the code. Let me know if you have any questions about them.

[samcoe]

Rather than introduce the TextMatch boolean to the Query struct I think we could make this simpler by just checking that the query kind is search.KindCode.

[joshkraft]

I like this idea. I will refactor this to leverage search.KindCode

[samcoe]

This example does not seem to add much as the example above shows off the same functionality. Perhaps we should change this one to show off a different flag?

[joshkraft]

I initially added this to highlight that using --owner works with both organizations and users, but looking at it now it does indeed seem redundant! Dropping this example.

[samcoe]

We recently started allowing -R as a shorthand for --repo flag in search commands, let's add that here.

[joshkraft]

Done 👍

[samcoe]

Let's skip the -l shorthand for now, if we want to add it I would also like to add it to the search repos command since it has an identical flag.

[joshkraft]

Done 👍

[samcoe]

I think what you have is solid, and getHighlightIndices is pretty clever. I took a quick pass at making it a bit simpler and idiomatic, let me know what you think:

func displayResults(io *iostreams.IOStreams, results search.CodeResult, limit int) error {
	cs := io.ColorScheme()
	tp := tableprinter.New(io)
	displayed := 0
outer:
	for _, code := range results.Items {
		if io.IsStdoutTTY() {
			header := fmt.Sprintf("%s %s", cs.GreenBold(code.Repo.FullName), cs.GreenBold(code.Path))
			tp.AddField(header)
			tp.EndRow()
		}
		for _, match := range code.TextMatches {
			fragments := formatMatch(cs, match)
			for i, fragment := range fragments {
				if io.IsStdoutTTY() {
					tp.AddField(fmt.Sprintf("%s. %s", strconv.Itoa(i+1), fragment))
				} else {
					tp.AddField(fmt.Sprintf("%s %s: %s", code.Repo.FullName, code.Path, fragment))
				}
				tp.EndRow()
				displayed++
				if displayed == limit {
					break outer
				}
			}
		}
	}
	if io.IsStdoutTTY() {
		header := fmt.Sprintf("Showing %d of %d results\n\n", displayed, max(displayed, results.Total))
		fmt.Fprintf(io.Out, "\n%s", header)
	}
	return tp.Render()
}

func formatMatch(cs *iostreams.ColorScheme, tm search.TextMatch) []string {
	shouldHighlight := matchHighlightMask(tm.Fragment, tm.Matches)
	var lines []string
	var found bool
	var b strings.Builder
	for i, c := range tm.Fragment {
		if c == '\n' {
			if found {
				lines = append(lines, b.String())
			}
			found = false
			b.Reset()
			continue
		}
		if shouldHighlight[i] {
			b.WriteString(cs.CyanBold(string(c)))
			found = true
		} else {
			b.WriteRune(c)
		}
	}
	return lines
}

func matchHighlightMask(fragment string, matches []search.Match) []bool {
	m := make([]bool, len(fragment))
	for _, match := range matches {
		start := match.Indices[0]
		stop := match.Indices[1]
		for i := start; i < stop; i++ {
			m[i] = true
		}
	}
	return m
}

[samcoe]

@joshkraft Thanks for making the requested changes. This looks great to me!

[mislav]

This looks splendid. Thanks @joshkraft for the hard work and @samcoe for thorough review.

There were a couple of deal-breakers for me, some that I have addressed in changes that I've just added:

• For some results, the code fragment wasn't rendered. It turned out that if the match is on the final line of the fragment that isn't terminated with \n, it would never get rendered. I've fixed it by ensuring that the final \n terminator is optional.
• Within a match itself, ANSI escape sequences would get output for each character for a match individually. I've changed this so that the only ANSI codes are printed at the start of the match and at the end of the match.
• Tableprinter was used, but the output is not a table. I switched to plain Fprintf statements.
• The color of the matched word was not standing out enough, so I've switched to a bright background color to provide more contrast for matches.
• The color of the repo name and the color of the filename was identical, making it tricky to distinguish between each other. I've changed the color of the repo name as an experiment for now.
• The printed numbers looked like line numbers, but did not correspond to the original line numbers from the files that contained matches, since the server doesn't seem to report this information. The fake line numbers for each fragment that always started at 1 were not useful so I removed them. Hopefully in the future we can get the information from the server about actual line numbers.
• Since we only print matching lines from the fragment returned from the server, preserving original indentation isn't paramount and can lead to visually scattered output. I've switched to trimming all indentation in the output.
• The header “Showing N of X results” was conflating numbers received from the server (total results) with numbers determined at render time (amount of line matches). When we display pagination info, the only data we ever use for that is server data, not client-side data. For simplicity, I've changed the header to display the number of items returned (len(result.Items)) and the total number of items, both directly taken from the API response payload.

What I feel still needs to be done:

• Figure out the line number situation so that we can finalize the output formats. After we ship this, it will be disruptive to change the output format, especially when it comes to changing the machine-readable output format.
• Agree on the final color scheme.
• Consider the accessibility of default output: if we don't rely on colors, is the textual output understandable enough? Would we need e.g. character separators between the repo name and file names other than a space?
• Expose the html_url value in the JSON output.
• Remove the redundant name value from JSON output since we have path.

@joshkraft There are no code changes that I'm requesting that you make to this PR at the moment; I think now our team needs to sync about this and make some decisions on how to roll this out.

[mislav]

Made some tweaks to this (most notably around polishing --json export formats) and I proclaim this shippable! Thanks for all the hard work @joshkraft. 🙏