gh gist delete: prompt for gist id #10154

danochoa · 2024-12-30T23:29:24Z

This changes gh gist delete to prompt the user to select a gist when no args are passed, instead of returning a message with usage info. And adds a confirm flag to prompt for confirmation. The code for prompting is already used by other commands (used gh gist view and gh repo delete for reference).

danochoa · 2025-01-02T19:33:39Z

Planning to add a prompt to confirm deletion as requested in #10073.

- pending confirmation re description in prompt

jtmcg

Deletion confirmation is the one change outside of the small UX change I discussed on the issue. I think it'll reduce the surface area of this change significantly!

pkg/cmd/gist/delete/delete.go

jtmcg · 2025-01-10T19:25:56Z

pkg/cmd/gist/delete/delete_test.go

+		opts         *DeleteOptions
+		cancel       bool
+		httpStubs    func(*httpmock.Registry)
+		mockGistList bool


Removing the prompter from this, re: the above comment, will make these tests much more readable I think. In fact, all the changes to deleteGist might just disappear with that change

Not sure I follow this part about removing the prompter here. Yes, moving the success confirmation code from deleteGist to deleteRun will result in no changes to deleteGist but not sure how that relates to mockGistList (renamed to mockPromptGists) which I think you’re saying to remove. I still added a call to PromptGists in deleteRun which requires the mock response provided by mockPromptGists in Test_deleteRun to test the added code paths right?

I could be missing something obvious but I used view_test.go as a reference and it seems like its doing the correct thing. I did notice that edit_test.go does not have tests for the PromptGists path, not sure why but maybe you're saying these tests aren't needed here?

I did find a couple of bugs in my tests which I have fixed for now. Open to refactor a bit if it’s mainly a readability issue, probably makes sense to DRY parts that I copied, if we don’t drop these tests.

Ah, no that's on me. I misread this and thought it was in Test_deleteGist and not Test_deleteRun. The former doesn't exist 😅 and the latter definitely needs the prompter mock. My bad!

🤔 Makes me think we should write tests for deleteGist, and I think to do that properly we'd want to move the HttpClient declaration in deleteRun into NewCmdDelete. The value of that is that we could inject a mocked httpClient into deleteRun and only stub the API calls in the deleteGist test instead of all the deleteRun tests.

That, however, is my personal mission in the tool and well outside the scope of this PR. For the sake of this, we can ignore these comments and leave it as is 🙂

jtmcg

Thanks for tackling this! It looks good and behaves as expected 👍

jtmcg · 2025-01-14T18:42:19Z

pkg/cmd/gist/delete/delete_test.go

+		opts         *DeleteOptions
+		cancel       bool
+		httpStubs    func(*httpmock.Registry)
+		mockGistList bool


Ah, no that's on me. I misread this and thought it was in Test_deleteGist and not Test_deleteRun. The former doesn't exist 😅 and the latter definitely needs the prompter mock. My bad!

🤔 Makes me think we should write tests for deleteGist, and I think to do that properly we'd want to move the HttpClient declaration in deleteRun into NewCmdDelete. The value of that is that we could inject a mocked httpClient into deleteRun and only stub the API calls in the deleteGist test instead of all the deleteRun tests.

That, however, is my personal mission in the tool and well outside the scope of this PR. For the sake of this, we can ignore these comments and leave it as is 🙂

jtmcg · 2025-01-14T22:28:59Z

pkg/cmd/gist/delete/delete_test.go

+	}
+}
+
+func Test_gistDelete(t *testing.T) {


I was tinkering on your branch, @danochoa, while chasing a few thoughts around this comment. These tests fell out of that tinkering and I thought I'd check them in, here.

I had a chat about my other thoughts with @andyfeller and we're going to table them for now 🙂

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).

danochoa added 2 commits December 30, 2024 15:41

gh gist delete: prompt for gist id

049332d

add gist delete tests for blank gist id

8f19d3b

danochoa requested a review from a team as a code owner December 30, 2024 23:29

danochoa requested a review from jtmcg December 30, 2024 23:29

cliAutomation added the external pull request originating outside of the CLI core team label Dec 30, 2024

danochoa added 2 commits December 30, 2024 18:18

rearrange struct vals for consistency

8390b41

undo extra move

61ffbe1

This comment was marked as spam.

Sign in to view

danochoa added 2 commits January 3, 2025 12:51

gist delete prompt for confirmation

f87d9f9

cleanup

75ace41

danochoa mentioned this pull request Jan 3, 2025

gh gist delete does not prompt for a gist to delete or prompt for confirmation before deletion #10073

Closed

danochoa added 8 commits January 3, 2025 13:54

Merge branch 'trunk' into gh-gist-delete-prompt

785cf43

tentative updates for acceptance criteria

a8f0aae

- pending confirmation re description in prompt

Merge branch 'trunk' into gh-gist-delete-prompt

7d1d261

gist delete prompt with gist description

9c65a32

fix gist description in tests

dccbc99

Merge branch 'trunk' into gh-gist-delete-prompt

928db3e

gist delete - use in confirmation and success info

7f71681

Merge branch 'trunk' into gh-gist-delete-prompt

d621b75

jtmcg suggested changes Jan 10, 2025

View reviewed changes

danochoa added 4 commits January 11, 2025 01:35

drop description from prompts, fix tests

5de282c

gist delete - check no gists

7885167

fix tests

08f7706

fix tests

1d9b4e4

danochoa requested a review from jtmcg January 13, 2025 19:17

danochoa and others added 2 commits January 13, 2025 21:17

Merge branch 'trunk' into gh-gist-delete-prompt

3370bc8

Add tests for deleteGist

3feeb9a

jtmcg approved these changes Jan 14, 2025

View reviewed changes

jtmcg merged commit 12ee8b7 into cli:trunk Jan 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gh gist delete: prompt for gist id #10154

gh gist delete: prompt for gist id #10154

Uh oh!

danochoa commented Dec 30, 2024 •

edited

Loading

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

danochoa commented Jan 2, 2025

Uh oh!

jtmcg left a comment

Uh oh!

Uh oh!

Uh oh!

jtmcg Jan 10, 2025

Uh oh!

danochoa Jan 11, 2025 •

edited

Loading

Uh oh!

jtmcg Jan 14, 2025

Uh oh!

jtmcg left a comment

Uh oh!

jtmcg Jan 14, 2025

Uh oh!

jtmcg Jan 14, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh gist delete: prompt for gist id #10154

gh gist delete: prompt for gist id #10154

Uh oh!

Conversation

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

Uh oh!

Uh oh!

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

danochoa commented Jan 2, 2025

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtmcg Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtmcg Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Uh oh!

jtmcg Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danochoa commented Dec 30, 2024 •

edited

Loading

danochoa Jan 11, 2025 •

edited

Loading

jtmcg Jan 14, 2025 •

edited

Loading