Supporting filtering on gist list
#9728
Conversation
|
@jtmcg this is ready. I rebased on |
|
I have a change nearly ready to highlight the search terms in the displayed text, which really only includes the description. This refactors the highlighting in I wanted to wait to finish it because:
|
jtmcg
left a comment
There was a problem hiding this comment.
Looks good! I do like the idea of syntax highlighting, but you're right - it may be awkward without showing the filenames or text that is also being filtered on.
We had discussed that maybe there's an opportunity to add a new type of TTY output to allow for showing files and their text, but perhaps that's a follow-up PR.
Anyway, I've really only got one comment that I've pushed a commit on 🙌 Let me know your thoughts on it
heaths
left a comment
There was a problem hiding this comment.
Change looks good considering the comment already seems to imply that:
Include gists' file content when filtering
The thought had crossed my mind as well when I was looking at
This could be default if ...though, given this is now considered a filter, I wonder if that would be too jarring. You're filtering the list, so one might expect the same table to be rendered. To clarify, I wasn't married to But even as implemented in this PR now, it's incredibly helpful. I tend to prototype a lot of ideas for the Azure SDKs for Rust I'm heading up now, and instead of letting https://play.rust-lang.org create a gist I can't change, I make my own along with a comment to open it in the playground (something I wish https://play.go.dev would support). I have tons of gists with |
|
If desired, I have a change ready that would output something like this:
Should we print this same thing if output is not to a TTY? Comparatively, |
I like the gist description being included during TTY and don't see it is too disruptive to the existing experience in I don't know if it makes sense breaking the non-TTY format of gh search code retry --owner cli | cat
So my knee jerk would be leaving out the description in non-TTY for now 🤷 |
andyfeller
left a comment
There was a problem hiding this comment.
Having a chance to discuss the feature outside of the code changes, I think things look pretty solid so far. While waiting on the highlighting change alluded to, here are a few minor suggestions.
Thank you as always, @heaths, for continuing to improve gh ❤️
The gist without a description - and just a single matching line - is pretty useless, IMO. I have tons of gist with just a That said, I don't see any big reason to keep this non-TTY formatting. It's not truly
This looks much more readable to me. (I also see I should probably format non-printable characters similar to what |
@andyfeller the changes are nearly ready in heaths#43. All I have to do is update tests but wanted to to wait on that (testing ANSI escape sequences spread out all over is time-consuming), I wanted to make sure we're settled on a course of action. I still have some doubts whether drastically and implicitly changing the format just by passing |
Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. @probablycorey @matschaffer @digitalfu cli#9745 cli#9721 cli#9728 cli#9746 №accuweaty24 #
I like the opt-in idea. Why not introduce a flag for the alternative view? Something like This could also be a follow-up PR, so we can get this one in and make the QoL visual improvements next |
|
Yeah, "verbose" doesn't sound right here; however, some variation on "view" makes sense. Let me review other commands for some inspiration. Maybe just I'd love to get @andyfeller's clarification on what he meant. I took it as not adding the description to what He was saying do exactly what However,
Assuming I get time, I'll fix up my other PR to this one (or if you want to merge, then I can rebase the other PR) this weekend and proceed down the |
|
Another idea came to mind: what if the default table view highlighted in the description or Or, what about Here's a mock up of
|
|
I'm kinda liking this idea of showing 
You can see examples of the existing behavior, highlighting the table to TTY and non-TTY, and highlighting content to TTY and non-TTY (using We don't need to add a new switch and I think the context makes sense. Thoughts? I went ahead and merged heaths#43 so that if you agree this is best, it's ready to merge. Effectively, |
Resolves cli#9704
Filter as we get results instead of getting them all. This allows us to more easily terminate pagination when opts.Limit is reached.
Resolves feedback in issue cli#9704
Also changes `--filter` argument to "expression" to coincide with some other commands' help text e.g., `--jq`.
Filtering can take a while, so show progress. This also uncovered a bug that I wasn't checking if a filter was actually specified, resulting in a non-nil `opts.Filter`.
When `--filter` is passed, matches will be highlighted in the existing table. If file names match, the "n file(s)" cell will be highlighted. When `--include-content` is additionally passed, the file name, description, and/or content will be printed like `search code` with matches highlighted.
|
I'm fine with this approach, given |
That is fair. My suggestion was primarily based on 1) the perspective that
I'm down with this in all honesty.
I'm not worried so much about the different experience when passing
Again, I felt the non-TTY $ gh search code retry --owner cli | cat
cli/cli:pkg/cmd/release/shared/upload.go: func shouldRetry(err error) bool {
cli/cli:pkg/cmd/release/shared/upload.go: bo := backoff.NewConstantBackOff(retryInterval)
cli/cli:pkg/cmd/release/shared/upload.go: return backoff.Retry(func() error {
cli/cli:internal/codespaces/codespaces.go: err := backoff.Retry(func() error {
cli/gh-webhook:webhook/forward.go: // If the error is a server disconnect (1006), retry connecting
cli/cli:pkg/cmd/pr/create/create.go: return backoff.Retry(func() error {
cli/cli:pkg/cmd/pr/create/create.go: // Only retry if we have forked the repo else the push should succeed the first time.
cli/cli:pkg/cmd/attestation/verification/attestation.go: return nil, fmt.Errorf("no attestations found in the OCI registry. Retry the command without the --bundle-from-oci flag to check GitHub for the attestation")
cli/cli:git/client.go: message: fmt.Sprintf("unable to find git executable in PATH; please install %s before retrying", programName),
cli/cli:internal/codespaces/api/api.go: retryBackoff time.Duration
cli/cli:internal/codespaces/api/api.go: retryBackoff: 100 * time.Millisecond,
cli/cli:go.mod: github.com/hashicorp/go-retryablehttp v0.7.7 // indirect
cli/cli:pkg/cmd/repo/sync/sync.go: return fmt.Errorf("refusing to sync due to uncommitted/untracked local changes\ntip: use `git stash --all` before retrying the sync and run `git stash pop` afterwards")
cli/cli:pkg/cmd/repo/fork/fork.go: cloneDir, err := backoff.RetryWithData(func() (string, error) {
cli/cli:pkg/cmd/repo/create/create.go: } else if err := cloneWithRetry(opts, remoteURL, templateRepoMainBranch); err != nil {
cli/cli:pkg/cmd/repo/create/create.go: return backoff.Retry(func() error {
cli/cli:pkg/cmd/repo/fork/fork_test.go: name: "does not retry clone if error occurs and exit code is not 128",
cli/cli:pkg/cmd/attestation/verify/verify_test.go: require.ErrorContains(t, runVerify(&customOpts), "no attestations found in the OCI registry. Retry the command without the --bundle-from-oci flag to check GitHub for the attestation")
cli/cli:pkg/cmd/attestation/verify/verify_test.go: require.ErrorContains(t, runVerify(&customOpts), "no attestations found in the OCI registry. Retry the command without the --bundle-from-oci flag to check GitHub for the attestation")
cli/cli:pkg/cmd/repo/create/create_test.go: name: "noninteractive create from template with retry",
cli/cli:pkg/cmd/release/shared/upload_test.go: func Test_uploadWithDelete_retry(t *testing.T) {
cli/cli:pkg/cmd/release/shared/upload_test.go: retryInterval = 0
cli/cli:pkg/cmd/repo/sync/sync_test.go: errMsg: "refusing to sync due to uncommitted/untracked local changes\ntip: use `git stash --all` before retrying the sync and run `git stash pop` afterwards",
cli/cli:internal/codespaces/api/api_test.go: retryBackoff: 0, |
But what about this implementation? I like this because |
pkg/cmd/gist/list/list.go
Outdated
| this will be slower and increase the rate limit used. Instead of printing a table, | ||
| code will be printed with highlights. | ||
| For supported regular expression syntax, see https://pkg.go.dev/regexp/syntax |
There was a problem hiding this comment.
Not sure if I included the surrounding < > earlier, but these are needed to ensure that links are linkable in the CLI manual.
| For supported regular expression syntax, see https://pkg.go.dev/regexp/syntax | |
| For supported regular expression syntax, see <https://pkg.go.dev/regexp/syntax> |
There was a problem hiding this comment.
I commented about that above. They actually render as < and > in the usage docs to the terminal, though. I looked at a lot of links in the CLI and didn't see them either. IMO, prioritizing --help output to a terminal should be priority; your doc formatter can - and honestly should - be handling URLs anyway. Most do - even github.com's does. I know markdown-lint recommends <> around URLs but the original markdown spec from Daring Fireball doesn't necessitate them.
There was a problem hiding this comment.
Never mind. I just compared the results of:
grep -r '<https:' **/*.goWith:
grep -rP '(?<!<)https:' **/*.goAnd stand corrected. Fixing...
There was a problem hiding this comment.
@andyfeller wait, no, I was right: the <> do render to the terminal:
Part of the source of my confusion was that a URL at the bottom if --help on any command didn't wrap the URL. I found this in pkg/cmd/root/help.go:
helpEntries = append(helpEntries, helpEntry{"LEARN MORE", heredoc.Docf(`
Use %[1]sgh <command> <subcommand> --help%[1]s for more information about a command.
Read the manual at https://cli.github.com/manual
Learn about exit codes using %[1]sgh help exit-codes%[1]s
`, "`")})I'll update this text as you recommend so it can hopefully get merged soon, but I do think it's worth considering whether terminal output should have <> surrounding URLs - IMO, looks bad and certainly inconsistent with any other CLI - or fix whatever is rendering markdown for the CLI docs.
There was a problem hiding this comment.
I commented about that above. They actually render as
<and>in the usage docs to the terminal, though. I looked at a lot of links in the CLI and didn't see them either. IMO, prioritizing--helpoutput to a terminal should be priority; your doc formatter can - and honestly should - be handling URLs anyway. Most do - even github.com's does. I knowmarkdown-lintrecommends<>around URLs but the original markdown spec from Daring Fireball doesn't necessitate them.
I admit the gh help usage isn't consistent in every place URLs are emitted. There have been multiple efforts to clean up as much as we could consistently do within various commands, however it isn't complete as you allude to.
As for whether gh contextually formats URLs differently depending on 1) --help, 2) man, or 3) web manual, I think that is reasonable as a next step.
andyfeller
left a comment
There was a problem hiding this comment.
@jtmcg : once you get a chance to review, if you also approve, please feel free to merge 🙇
jtmcg
left a comment
There was a problem hiding this comment.
Overall, the code looks good. There's one improvement I would like to see, though, which I've commented on.
| matched := false | ||
| out.Reset() | ||
|
|
||
| if filename, err = highlightMatch(file.Filename, filter, &matched, cs.Green, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintln(out, cs.Blue(gist.ID), filename) | ||
|
|
||
| if gist.Description != "" { | ||
| if description, err = highlightMatch(gist.Description, filter, &matched, cs.Bold, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintf(out, "%s%s\n", tab, description) | ||
| } | ||
|
|
||
| if file.Content != "" { | ||
| for _, line := range strings.FieldsFunc(file.Content, split) { | ||
| if filter.MatchString(line) { | ||
| if line, err = highlightMatch(line, filter, &matched, normal, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintf(out, "%[1]s%[1]s%[2]s\n", tab, line) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if matched { | ||
| fmt.Fprintln(io.Out, out.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think I'd like to see this refactored out into a new function. Something like formatGistWithContent or similar. This confers a few benefits:
- Testing of that function is limited to a single gist, making it easier to test the various permutations implied by the conditionals
- It separates the responsibility of formatting the gists and formatting the list
- It makes it easier to read
Non-blocking: I'd also like to see the formatting code be more representative of what the output will look like, though I'm not exactly sure how to do that 😅 I find it hard to understand what the output will be from the code aside from reading the tests, and when that happens test expectations often become copy/pasted output. I generally have this complaint throughout the CLI, though, so I'm not going to block on it.
There was a problem hiding this comment.
It's going to be largely duplicative. The checks for things like Description or Content aren't just there for --filter --include-content. A gist - however it's printed - may not have a description. Or did you just mean lines 238 through 247 i.e., just for the if file.Content != "" conditional? Everything but that check would be duplicative and, not only increase maintenance costs, but likely increase binary size.
This is also taking advantage of a lot of singularly-defined variables including functions we'd have to pass in or keep allocating / redefining in each iteration like a strings.Builder that will be good to reuse to avoid unnecessary allocations. All that we'd have to pass in with enough parameters we'll likely exceed parameter registers on every platform and have to push arguments to the stack for each iteration.
There was a problem hiding this comment.
Let me cut a branch and tinker on it to show you what I'm thinking. You may be right, but I'm gonna give it a try anyway 🙂
There was a problem hiding this comment.
Alright, I tinkered around and I wasn't happy with the lowest hanging fruit. What I would like to see happen (well outside the scope of this) is for fetching and filtering to be hoisted way up the logic stack and set in NewCmdList, kind of how the IssuePrinter was extracted in this PR (merging the branch at a later date).
The problem is that this probably requires a rewrite of how we're fetching, too, and that is waaaay out of scope 😅
As a result, I'm fine to leave it as is in this PR. Thanks for indulging me and your patience while I dig into it myself 🍻
| matched := false | ||
| out.Reset() | ||
|
|
||
| if filename, err = highlightMatch(file.Filename, filter, &matched, cs.Green, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintln(out, cs.Blue(gist.ID), filename) | ||
|
|
||
| if gist.Description != "" { | ||
| if description, err = highlightMatch(gist.Description, filter, &matched, cs.Bold, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintf(out, "%s%s\n", tab, description) | ||
| } | ||
|
|
||
| if file.Content != "" { | ||
| for _, line := range strings.FieldsFunc(file.Content, split) { | ||
| if filter.MatchString(line) { | ||
| if line, err = highlightMatch(line, filter, &matched, normal, cs.Highlight); err != nil { | ||
| return err | ||
| } | ||
| fmt.Fprintf(out, "%[1]s%[1]s%[2]s\n", tab, line) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if matched { | ||
| fmt.Fprintln(io.Out, out.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Alright, I tinkered around and I wasn't happy with the lowest hanging fruit. What I would like to see happen (well outside the scope of this) is for fetching and filtering to be hoisted way up the logic stack and set in NewCmdList, kind of how the IssuePrinter was extracted in this PR (merging the branch at a later date).
The problem is that this probably requires a rewrite of how we're fetching, too, and that is waaaay out of scope 😅
As a result, I'm fine to leave it as is in this PR. Thanks for indulging me and your patience while I dig into it myself 🍻
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.58.0` -> `v2.59.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.59.0`](https://github.com/cli/cli/releases/tag/v2.59.0): GitHub CLI 2.59.0 [Compare Source](cli/cli@v2.58.0...v2.59.0) #### What's Changed - Allow community submitted design work by [@​BagToad](https://github.com/BagToad) in cli/cli#9683 - Improve `SECURITY.md` with expectations for privately reported vulnerabilities by [@​BagToad](https://github.com/BagToad) in cli/cli#9687 - Emit a log message when extension installation falls back to a `darwin-amd64` binary on an Apple Silicon macOS device by [@​timrogers](https://github.com/timrogers) in cli/cli#9650 - Print the login URL even when opening a browser by [@​ulfjack](https://github.com/ulfjack) in cli/cli#7091 - configurable maxwidth for markdown WithWrap() by [@​smemsh](https://github.com/smemsh) in cli/cli#9626 - Handle errors when parsing hostname in auth flow by [@​BagToad](https://github.com/BagToad) in cli/cli#9729 - Add `repo license list/view` and `repo gitignore list/view` by [@​BagToad](https://github.com/BagToad) in cli/cli#9721 - Introduce testscript acceptance tests generally, and for the MR command specifically by [@​williammartin](https://github.com/williammartin) in cli/cli#9745 - Support `GH_ACCEPTANCE_SCRIPT` env var to target a single script by [@​williammartin](https://github.com/williammartin) in cli/cli#9756 - Ensure Acceptance defer failures are debuggable by [@​williammartin](https://github.com/williammartin) in cli/cli#9754 - Add acceptance task to makefile by [@​williammartin](https://github.com/williammartin) in cli/cli#9748 - Add Acceptance tests for `issue` command by [@​williammartin](https://github.com/williammartin) in cli/cli#9757 - Update IsEnterprise and IsTenancy for orthogonality using go-gh by [@​jtmcg](https://github.com/jtmcg) in cli/cli#9755 - Supporting filtering on `gist list` by [@​heaths](https://github.com/heaths) in cli/cli#9728 #### New Contributors - [@​ulfjack](https://github.com/ulfjack) made their first contribution in cli/cli#7091 - [@​smemsh](https://github.com/smemsh) made their first contribution in cli/cli#9626 **Full Changelog**: cli/cli@v2.58.0...v2.59.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Resolves #9704