Fix: `gh gist view` and `gh gist edit` prompts with no TTY #10048

mateusmarquezini · 2024-12-09T11:38:36Z

…o fix/issue_10042

… TTY is detected

BagToad

Thank you for your patience with this one @mateusmarquezini ✨

I wasn't sure if you're ready for review on this yet per your PR description, but I've gone ahead and added some comments in case that is useful.

I think the implementation is sound, but I think we should improve the tests to align more with how some other commands are testing for this interactivity error message - I've put more details about that in the PR comments.

Please let me know if you have any questions or when you're ready for another review ❤️

pkg/cmd/gist/edit/edit.go

pkg/cmd/gist/view/view_test.go

mateusmarquezini · 2025-01-10T20:19:58Z

Hey @BagToad! Thanks so much for your amazing review & suggestions! 🌟

Oops, I forgot to update the PR description—thanks for the heads up!

About your other comments—awesome! I'll apply those suggestions and ping you once it's ready for another review!

Have an awesome weekend! ✨

Update pkg/cmd/gist/edit/edit.go after code review Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com>

…ages when TTY is unavailable

mateusmarquezini · 2025-01-13T16:53:45Z

Hey @BagToad! Thanks again for all your suggestions—they were super helpful and really guided me through the corrections!✨

I've got some questions about the fixes in gist/edit/edit_test.go. Maybe you can help me out!

So, I tried applying the same approach I used in view_test.go (example) to edit_test.go (example), as it'd be great to test the scenario where TTY isn't available.

However, I'm running into this error and feeling a bit stuck on the right way to fix this specific test.

Looking at the code, it seems the issue happens because the test runner always sets tt.opts.Selector = "1234' (example), which makes it reach the API call instead of failing early. Fixing this would require some additional changes I wasn't expecting 😂, especially in the HTTP stubs for edit_test.go.

My question is: Should I keep going with this approach, or do you think there's an easier way to handle this?

Additionally, feel free to give me some feedback on the test I already created in pkg/cmd/gist/view/view_test.go, since, as I mentioned, I'm applying a similar test in gist/edit/edit_test.go and I'd love your feedback before moving forward.

Thanks in advance for your input! 🌟

This changes the gist edit tests to use the positive `istty` instead of the previous inverse `nontty`, which is consistent with the way other commands are written.

BagToad · 2025-01-16T18:01:07Z

Hey @mateusmarquezini! 👋

However, I'm running into this error and feeling a bit stuck on the right way to fix this specific test.

Looking at the code, it seems the issue happens because the test runner always sets tt.opts.Selector = "1234' (example), which makes it reach the API call instead of failing early. Fixing this would require some additional changes I wasn't expecting 😂, especially in the HTTP stubs for edit_test.go.

My question is: Should I keep going with this approach, or do you think there's an easier way to handle this?

I see what you mean! These gist edit tests don't appear to be consistent with other commands in the CLI, especially gist view, but they really should be.

So, to save us the headache trying to work around that inconsistency, I'm going to push some commits to this PR to fix up the tests, make them easier to read, and hopefully more consistent - I hope that is okay with you 😃

I'm also resolving some merge conflicts that popped up 😜

BagToad · 2025-01-16T18:40:32Z

Looking at the code, it seems the issue happens because the test runner always sets tt.opts.Selector = "1234' (example), which makes it reach the API call instead of failing early. Fixing this would require some additional changes I wasn't expecting 😂, especially in the HTTP stubs for edit_test.go.

To be more specific with what I changed in regard to this - I removed that hardcoded selector assignment that was causing us problems and instead added it to the tests that require it. It's a bit more repetitive, but I do not currently see a better way of doing it because we have a situation where we want the selector to be nothing 😁

I added comments to some of the HTTP stubs and parameter checking that should hopefully make this easier to follow. I also renamed some of the table test variables to make it clearer what they're meant for.

I also added some extra PR comments to help you, me, and future readers understand what's going on in these tests. Feel free to click "resolve" on those comments at your discretion 👍

pkg/cmd/gist/edit/edit_test.go

BagToad · 2025-01-16T18:53:15Z

Additionally, feel free to give me some feedback on the test I already created in pkg/cmd/gist/view/view_test.go, since, as I mentioned, I'm applying a similar test in gist/edit/edit_test.go and I'd love your feedback before moving forward.

Looks good to me! I'm sorry that gist edit wasn't as straightforward 😆

@mateusmarquezini I'll let you have a look over the changes I've pushed - if you have any questions, let me know. Otherwise, let me know and I'll do a last review 🎉

mateusmarquezini · 2025-01-19T14:35:55Z

Thank you so much for the amazing help you provided @BagToad!❤️

I honestly didn't expect all those changes in the gist edit haha—thank you so much for your contribution to this PR!

Feel free to go ahead and make a final review on it 🎉

BagToad

LGTM 🚀

❯ ./bin/gh gist view | cat
gist ID or URL required when not running interactively

Usage:  gh gist view [<id> | <url>] [flags]

Flags:
  -f, --filename string   Display a single file from the gist
      --files             List file names from the gist
  -r, --raw               Print raw instead of rendered gist contents
  -w, --web               Open gist in the browser
  
❯ ./bin/gh gist edit | cat
gist ID or URL required when not running interactively

Usage:  gh gist edit {<id> | <url>} [<filename>] [flags]

Flags:
  -a, --add string        Add a new file to the gist
  -d, --desc string       New description for the gist
  -f, --filename string   Select a file to edit
  -r, --remove string     Remove a file from the gist

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.65.0` -> `v2.66.1` | 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.66.1`](https://github.com/cli/cli/releases/tag/v2.66.1): GitHub CLI 2.66.1 [Compare Source](cli/cli@v2.66.0...v2.66.1) #### Hotfix: `gh pr view` fails with provided URL This addresses a regression in `gh pr view` was reported in [#10352](cli/cli#10352). This regression was due to a change in `v2.66.0` that no longer allowed `gh pr` subcommands to execute properly outside of a git repo. #### What's Changed - Hotfix: `gh pr view` fails with provided URL by [@jtmcg](https://github.com/jtmcg) in cli/cli#10354 **Full Changelog**: cli/cli@v2.66.0...v2.66.1 ### [`v2.66.0`](https://github.com/cli/cli/releases/tag/v2.66.0): GitHub CLI 2.66.0 [Compare Source](cli/cli@v2.65.0...v2.66.0) #### `gh pr view` and `gh pr status` now respect common triangular workflow configurations Previously, `gh pr view` and `gh pr status` would fail for pull request's (MR) open in triangular workflows. This was due to `gh` being unable to identify the MR's corresponding remote and branch refs on GitHub. Now, `gh pr view` and `gh pr status` should successfully identify the MR's refs when the following common git configurations are used: - [`branch.<branchName>.pushremote`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtpushRemote) is set - [`remote.pushDefault`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-remotepushDefault) is set Branch specific configuration, the former, supersedes repo specific configuration, the latter. Additionally, if the [`@{push}` revision syntax](https://git-scm.com/docs/gitrevisions#Documentation/gitrevisions.txt-emltbranchnamegtpushemegemmasterpushemempushem) for git resolves for a branch, `gh pr view` and `gh pr status` should work regardless of additional config settings. For more information, see - cli/cli#9363 - cli/cli#9364 - cli/cli#9365 - cli/cli#9374 #### `gh secret list`, `gh secret set`, and `gh secret delete` now require repository selection when multiple `git` remotes are present Previously, `gh secret list`, `gh secret set`, and `gh secret delete` would determine which remote to target for interacting with GitHub Actions secrets. Remotes marked as default using `gh repo set-default` or through other `gh` commands had higher priority when figuring out which repository to interact with. This could have unexpected outcomes when using `gh secret` commands with forked repositories as the upstream repository would generally be selected. Now, `gh secret` commands require users to disambiguate which repository should be the target if multiple remotes are present and the `-R, --repo` flag is not provided. For more information, see cli/cli#4688 #### Extension update notices now notify once every 24 hours per extension and can be disabled Previously, the GitHub CLI would notify users about newer versions every time an extension was executed. This did not match GitHub CLI notices, which only notified users once every 24 hours and could be disabled through an environment variable. Now, extension update notices will behave similar to GitHub CLI notices. To disable extension update notices, set the `GH_NO_EXTENSION_UPDATE_NOTIFIER` environment variable. For more information, see cli/cli#9925 #### What's Changed ##### ✨ Features - Draft for discussing testing around extension update checking behavior by [@andyfeller](https://github.com/andyfeller) in cli/cli#9985 - Make extension update check non-blocking by [@andyfeller](https://github.com/andyfeller) in cli/cli#10239 - Ensure extension update notices only notify once within 24 hours, provide ability to disable all extension update notices by [@andyfeller](https://github.com/andyfeller) in cli/cli#9934 - feat: make the extension upgrade fancier by [@nobe4](https://github.com/nobe4) in cli/cli#10194 - fix: padded display by [@nobe4](https://github.com/nobe4) in cli/cli#10216 - Update `gh attestation` attestation bundle fetching logic by [@malancas](https://github.com/malancas) in cli/cli#10185 - Require repo disambiguation for secret commands by [@williammartin](https://github.com/williammartin) in cli/cli#10209 - show error message for rerun workflow older than a month ago by [@iamrajhans](https://github.com/iamrajhans) in cli/cli#10227 - Update `gh attestation verify` table output by [@malancas](https://github.com/malancas) in cli/cli#10104 - Enable MSI building for Windows arm64 by [@dennisameling](https://github.com/dennisameling) in cli/cli#10297 - feat: Add support for creating autolink references by [@hoffm](https://github.com/hoffm) in cli/cli#10180 - Find MRs using `@{push}` by [@Frederick888](https://github.com/Frederick888) in cli/cli#9208 - feat: Add support for viewing autolink references by [@hoffm](https://github.com/hoffm) in cli/cli#10324 - Update `gh attestation` bundle fetching logic by [@malancas](https://github.com/malancas) in cli/cli#10339 ##### 🐛 Fixes - gh gist delete: prompt for gist id by [@danochoa](https://github.com/danochoa) in cli/cli#10154 - Better handling for waiting for codespaces to become ready by [@cmbrose](https://github.com/cmbrose) in cli/cli#10198 - Fix: `gh gist view` and `gh gist edit` prompts with no TTY by [@mateusmarquezini](https://github.com/mateusmarquezini) in cli/cli#10048 - Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` by [@jtmcg](https://github.com/jtmcg) in cli/cli#10197 - Add job to deployment workflow to validate the tag name for a given release by [@jtmcg](https://github.com/jtmcg) in cli/cli#10121 - \[gh run list] Stop progress indicator on failure from `--workflow` flag by [@iamazeem](https://github.com/iamazeem) in cli/cli#10323 - Update deployment.yml by [@andyfeller](https://github.com/andyfeller) in cli/cli#10340 ##### 📚 Docs & Chores - Add affected version heading to bug report issue form by [@BagToad](https://github.com/BagToad) in cli/cli#10269 - chore: fix some comments by [@petercover](https://github.com/petercover) in cli/cli#10296 - Update triage.md to reflect FR experiment outcome by [@jtmcg](https://github.com/jtmcg) in cli/cli#10196 - Clear up --with-token fine grained PAT usage by [@williammartin](https://github.com/williammartin) in cli/cli#10186 - Correct help documentation around template use in `gh issue create` by [@andyfeller](https://github.com/andyfeller) in cli/cli#10208 - chore: fix some function names in comment by [@zhuhaicity](https://github.com/zhuhaicity) in cli/cli#10225 - Tiny typo fix by [@robmorgan](https://github.com/robmorgan) in cli/cli#10265 - add install instructions for Manjaro Linux by [@AMS21](https://github.com/AMS21) in cli/cli#10236 - Update test to be compatible with latest Glamour v0.8.0 by [@ottok](https://github.com/ottok) in cli/cli#10151 - Add more `gh attestation verify` integration tests by [@malancas](https://github.com/malancas) in cli/cli#10102 #####

Dependencies - Bump github.com/mattn/go-colorable from 0.1.13 to 0.1.14 by [@dependabot](https://github.com/dependabot) in cli/cli#10215 - Bump github.com/sigstore/protobuf-specs from 0.3.2 to 0.3.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10214 - Bump github.com/gabriel-vasile/mimetype from 1.4.7 to 1.4.8 by [@dependabot](https://github.com/dependabot) in cli/cli#10184 - Bump google.golang.org/protobuf from 1.36.2 to 1.36.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10250 - Bump golangci-linter and address failures to prepare for Go 1.24 strictness by [@mikelolasagasti](https://github.com/mikelolasagasti) in cli/cli#10279 - Bump github.com/google/go-containerregistry from 0.20.2 to 0.20.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10257 - Bump actions/attest-build-provenance from 2.1.0 to 2.2.0 by [@dependabot](https://github.com/dependabot) in cli/cli#10300 - Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 by [@dependabot](https://github.com/dependabot) in cli/cli#10306 - Upgrade sigstore-go to v0.7.0: fixes [#10114](cli/cli#10114) formatting issue by [@codysoyland](https://github.com/codysoyland) in cli/cli#10309 - Bump github.com/in-toto/attestation from 1.1.0 to 1.1.1 by [@dependabot](https://github.com/dependabot) in cli/cli#10319 #### New Contributors Big thank you to our many new *and* longtime contributors making this release happen!! ❤️ ✨ - [@zhuhaicity](https://github.com/zhuhaicity) made their first contribution in cli/cli#10225 - [@danochoa](https://github.com/danochoa) made their first contribution in cli/cli#10154 - [@robmorgan](https://github.com/robmorgan) made their first contribution in cli/cli#10265 - [@iamrajhans](https://github.com/iamrajhans) made their first contribution in cli/cli#10227 - [@AMS21](https://github.com/AMS21) made their first contribution in cli/cli#10236 - [@petercover](https://github.com/petercover) made their first contribution in cli/cli#10296 - [@ottok](https://github.com/ottok) made their first contribution in cli/cli#10151 - [@dennisameling](https://github.com/dennisameling) made their first contribution in cli/cli#10297 - [@iamazeem](https://github.com/iamazeem) made their first contribution in cli/cli#10323 - [@Frederick888](https://github.com/Frederick888) made their first contribution in cli/cli#9208 **Full Changelog**: cli/cli@v2.65.0...v2.66.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. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

mateusmarquezini added 6 commits December 9, 2024 08:35

fixing gh gist view prompts with no TTY

cbaaf77

Merge branch 'trunk' into fix/issue_10042

a3cf4e1

cli#10042 fixed test

b740486

Merge branch 'fix/issue_10042' of github.com:mateusmarquezini/cli int…

a7dceb3

…o fix/issue_10042

Merge branch 'trunk' into fix/issue_10042

a3a283c

cli#10042: Add error messages for 'gh gist view/edit' prompts when no…

f4f8840

… TTY is detected

mateusmarquezini marked this pull request as ready for review January 9, 2025 16:11

mateusmarquezini requested a review from a team as a code owner January 9, 2025 16:11

mateusmarquezini requested a review from BagToad January 9, 2025 16:11

cliAutomation added the external pull request originating outside of the CLI core team label Jan 9, 2025

cli#10042 removed unnecessary field

4c2ba56

BagToad changed the title ~~Fixing gh gist view prompts with no TTY~~ Fix: gh gist view and gh gist edit prompts with no TTY Jan 10, 2025

BagToad requested changes Jan 10, 2025

View reviewed changes

pkg/cmd/gist/edit/edit.go Show resolved Hide resolved

pkg/cmd/gist/view/view_test.go Outdated Show resolved Hide resolved

mateusmarquezini and others added 6 commits January 12, 2025 14:50

Merge branch 'trunk' into fix/issue_10042

f65ac16

Update pkg/cmd/gist/edit/edit.go

c5d6ae6

Update pkg/cmd/gist/edit/edit.go after code review Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com>

removed unnecessary space

24e9fed

cli#10042: Applied code review suggestions to view_test.go

977e232

cli#10042: Attempt to add a new test scenario for handling error mess…

c0f6eb0

…ages when TTY is unavailable

Merge branch 'trunk' into fix/issue_10042

4eb0e81

BagToad self-assigned this Jan 16, 2025

Fix: Gist edit tests for interactivity

35d81e3

This changes the gist edit tests to use the positive `istty` instead of the previous inverse `nontty`, which is consistent with the way other commands are written.

BagToad added 3 commits January 16, 2025 11:06

Merge 'trunk' into fix/issue_10042

a2dce58

Fix: gist edit/view tests var name consistency

dec4667

Fix: gist edit test name

5b6fd53

Fix: accidental whitespace in gist edit

facd0ca

BagToad reviewed Jan 16, 2025

View reviewed changes

pkg/cmd/gist/edit/edit_test.go Show resolved Hide resolved

BagToad reviewed Jan 16, 2025

View reviewed changes

pkg/cmd/gist/edit/edit_test.go Show resolved Hide resolved

mateusmarquezini requested a review from BagToad January 19, 2025 14:36

BagToad approved these changes Jan 20, 2025

View reviewed changes

BagToad merged commit 722fc67 into cli:trunk Jan 20, 2025

mateusmarquezini deleted the fix/issue_10042 branch January 20, 2025 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: `gh gist view` and `gh gist edit` prompts with no TTY #10048

Fix: `gh gist view` and `gh gist edit` prompts with no TTY #10048

mateusmarquezini commented Dec 9, 2024 •

edited

Loading

Uh oh!

BagToad left a comment

Uh oh!

Uh oh!

Uh oh!

mateusmarquezini commented Jan 10, 2025 •

edited

Loading

Uh oh!

mateusmarquezini commented Jan 13, 2025

Uh oh!

BagToad commented Jan 16, 2025 •

edited

Loading

Uh oh!

BagToad commented Jan 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

BagToad commented Jan 16, 2025

Uh oh!

mateusmarquezini commented Jan 19, 2025 •

edited

Loading

Uh oh!

BagToad left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix: gh gist view and gh gist edit prompts with no TTY #10048

Fix: gh gist view and gh gist edit prompts with no TTY #10048

Conversation

mateusmarquezini commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BagToad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mateusmarquezini commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateusmarquezini commented Jan 13, 2025

Uh oh!

BagToad commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BagToad commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BagToad commented Jan 16, 2025

Uh oh!

mateusmarquezini commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BagToad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix: `gh gist view` and `gh gist edit` prompts with no TTY #10048

Fix: `gh gist view` and `gh gist edit` prompts with no TTY #10048

mateusmarquezini commented Dec 9, 2024 •

edited

Loading

mateusmarquezini commented Jan 10, 2025 •

edited

Loading

BagToad commented Jan 16, 2025 •

edited

Loading

BagToad commented Jan 16, 2025 •

edited

Loading

mateusmarquezini commented Jan 19, 2025 •

edited

Loading