Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` #10197

jtmcg · 2025-01-07T21:50:06Z

Motivation

After some discussion with @williammartin, we agreed that the naked return values pattern is something that we'd like to avoid in our codebase and that we should make an effort to remove them when we encounter them. This is the PR to remove the named return values from ReadBranchConfig that I encountered while working through #10188.

I also refactored ReadBranchConfig for two reasons: to surface the previously hidden errors it could produce and to allow for better test coverage of its core functionality. I did the latter by separating out the git call and the BranchConfig struct creation into their own functions to leverage dependency injection of the git call's return value into the BranchConfig creation.

While making this refactor work with the rest of the tool, I stumbled upon more named return values in prSelectorForCurrentBranch and decided to remove these as well. This resulted in a slight reorganization of the function in a way that I believe makes it easier to follow.

Changes

Remove named returns from ReadBranchConfig and surface errors
Refactor ReadBranchConfig for test coverage of newly returned erros
Remove named return values from prSelectorForCurrentBranch

Testing

I haven't touched any of the original tests besides accommodating the new ReadBranchConfig api, and they are all passing, so I'm decently confident that this won't introduce any breaking changes. However, to exercise all the changes, follow these steps:

Pull down the branch gh pr checkout 10197
Build the branch
Execute the gh pr status command

BagToad · 2025-01-07T21:59:36Z

I'd be interested in hearing any key points from that discussion for my own understanding :)

Is this a design choice we're making universally or is there some qualifier that would make named return values appropriate for some situations in the CLI?

jtmcg · 2025-01-07T23:08:07Z

I'd be interested in hearing any key points from that discussion for my own understanding :)

The conversation was here in slack. The tl;dr of it is that named return values and the corresponding "naked" returns obscure information and can make code hard to grok. I explain what was confusing about ReadBranchConfig in the thread, but I'll add that this was silently handling errors, which wasn't super clear. To demonstrate what I mean:

// This function returns the error
func errorReturningFunction() (result Result, err error) {
    _, err = someOtherFunc()
    if err != nil {
        return // equivalent to `return Result{}, err` with unnamed returns
    } 
}

// This function doesn't return an error
func silentErrorFunction() (result Result) {
    _, err = someOtherFunc()
    if err != nil {
        return // equivalent to `return Result{}` with unnamed returns
    } 
}

These two functions look nearly identical except for the return signature in the function declaration. Now imagine this return is 100 lines down 😅

Is this a design choice we're making universally or is there some qualifier that would make named return values appropriate for some situations in the CLI?

I'd advocate for yes, this should be universal. The only time I could think of named returns being ok is for really short functions, and even then there's no real benefit. If that function were to grow, we'd be in a bad state

williammartin · 2025-01-08T09:38:21Z

I'd advocate for yes, this should be universal. The only time I could think of named returns being ok is for really short functions, and even then there's no real benefit. If that function were to grow, we'd be in a bad state

Firstly, let's make a distinction between named returns and naked returns. The latter depends on the former, but you can also use named returns specifically. Named returns result in the initialisation of the variable upon function entry with the zero value.

func sum(nums []int) (result int) {
    for _, n := range nums {
      result = result + n
     }
    return result // <--- still is explicit about the variable being returned

I don't believe there are any cases in which naked returns are the right option and I would advocate that they should all be removed.

Named returns however, do facilitate some patterns that can't be achieved otherwise, such as error handling in a defer block:

func fallible() error {}
func cleanup() error {}

func doThing() err {
    defer func() {
        cleanup() <--- uh oh, cleanup can also fail but we lose the error
    }()
    
   return fallible()
}

In this example the obvious thing to do is reorganise things to cleanup only happens on error, but hopefully it doesn't take too much imagination to think of a scenario where many operations can fail and if any of them fail we need to cleanup (or perhaps we need to recover a panic.

Thus:

func fallible() error {}
func cleanup() error {}

func doThing() (err error) {
    defer func() {
        if err != nil {           
            err = errors.Join(err, cleanup()) // <--- now we join (or ignore if `nil`) the cleanup error to have that context
        }
    }()
    
   err = fallible()
   return err
}

Hope that makes sense.

In my opinion:

No naked returns
Named returns only in pursuit of some specific patterns. There better be a clear reason for it.

williammartin

Thanks for cleaning up as you go!

git/client.go

git/client_test.go

pkg/cmd/pr/create/create.go

pkg/cmd/pr/status/status.go

…rom Client I think I went too far with my previous refactor and am backing out of it. Adding a private readBranchConfig method on the client wasn't providing any real additional value, so I've put it back into ReadBranchConfig. However, I think there is still value in having parseBranchConfig (formerly createBranchConfig) as a separate util function, as it both improves readability of ReadBranchConfig and makes parsing its purpose easier through the bespoke tests for it.

Replace the git config argument in prSelectorForCurrentBranch with the branchConfig it was used to fetch. The tests needed to be refactored accordingly to support this change to the prSelectorForCurrentBranch API. In addition, I've moved the test to a table test format so I can expand the test coverage in the next commit.

…actor

… tests

jtmcg · 2025-01-08T23:25:43Z

Great review, @williammartin! I appreciate the balance of revealing nuance in your comments but asking questions that led me to figuring it out... Learned a lot, here 🙌

I think the PR is probably close, now, but I'm looking for your thoughts on the error return for ReadBranchConfig, re: this comment.

andyfeller

Quite a bit of extensive work, but I appreciate chipping away at named returns! 🙌

There are a few sections I had questions while a few others that raised more meta questions regarding content we use in the tests.

git/client.go

git/client_test.go

pkg/cmd/pr/create/create.go

pkg/cmd/pr/create/create_test.go

pkg/cmd/pr/shared/finder.go

pkg/cmd/pr/status/status_test.go

cmd.Output() will return an error when the git command ran successfully but had no output. To handle this, we can check Stderr, as we expect it to be populated for any ExitErrors or otherwise when there is a command failure. This allows for propagation of this error handling up the call chain, so we are now returning errors if the call to git fails instead of just handing off an empty BranchConfig and suppressing the errors. Additionally, I've removed some more naked returns that I found in pkg/cmd/pr/create.go createRun

I've only added the one test for parseCurrentBranch because the function appears to be largely exercised by TestFind. There's definitely an opportunity for a bigger refactor of the tests, here, but I want to avoid scope creep as I propagate the ReadBranchConfig api changes throughout the codebase

git/client.go

Before this refactor, the errors emitted by ghrepo.FromURL and rem.FindName() were suppressed. It isn't clear whether this was intentional or not, but we've made the decision here to maintain the original error behavior while still refactoring the return values for more clarity. I've left a comment at each error handling block to explain this decision. Additionally, I've added the necessary git command stubs to the other tests in status_test.go so that the tests are now passing.

andyfeller

LGTM ✨ That said, will leave it to you, @jtmcg, if you want to wait on final review from Will 👍

williammartin

LGTM.

➜  cli git:(jtmcg/remove-named-returns) set -o pipefail && GH_ACCEPTANCE_HOST=github.com GH_ACCEPTANCE_ORG=gh-acceptance-testing go test -tags acceptance -json -coverprofile=coverage.out -coverpkg=./... -run ^TestPullRequests$ github.com/cli/cli/v2/acceptance | tparse --all go test
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │                        TEST                        │             PACKAGE               │
│─────────┼─────────┼────────────────────────────────────────────────────┼───────────────────────────────────│
│  PASS   │   13.73 │ TestPullRequests/pr-create-from-issue-develop-base │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   13.35 │ TestPullRequests/pr-checkout-with-url-from-fork    │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   13.32 │ TestPullRequests/pr-merge-rebase-strategy          │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   13.24 │ TestPullRequests/pr-merge-merge-strategy           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   11.04 │ TestPullRequests/pr-view-same-org-fork             │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   10.57 │ TestPullRequests/pr-create-with-metadata           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   10.10 │ TestPullRequests/pr-create-basic                   │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   10.09 │ TestPullRequests/pr-create-from-manual-merge-base  │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    9.94 │ TestPullRequests/pr-view                           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    9.09 │ TestPullRequests/pr-list                           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    8.43 │ TestPullRequests/pr-create-without-upstream-config │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    8.34 │ TestPullRequests/pr-checkout-by-number             │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    7.99 │ TestPullRequests/pr-comment                        │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    7.27 │ TestPullRequests/pr-checkout                       │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    0.00 │ TestPullRequests                                   │ github.com/cli/cli/v2/acceptance  │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
┌────────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │             PACKAGE              │ COVER │ PASS │ FAIL │ SKIP  │
│─────────┼─────────┼──────────────────────────────────┼───────┼──────┼──────┼───────│
│  PASS   │ 25.23s  │ github.com/cli/cli/v2/acceptance │ 15.7% │  15  │  0   │  0    │
└────────────────────────────────────────────────────────────────────────────────────┘

williammartin · 2025-01-13T13:02:28Z

I think this is probably going to cause merge conflicts with https://github.com/cli/cli/pull/9208/files#diff-ccedca01e076cd065987253b366873ca2bee2e28c9d0bc13d50d7b7795c0bf4eR380-R420

Let's fix this for @Frederick888. Getting that PR through is next on our backlog too, so maybe I'll just do it.

A recent refactor caused the API for ReadBranchConfig to change, resulting in changes for its consumers. Additionally, there was a large refactor of the tests associated with ReadBranchConfig and its consumers to gain confidence in this refactor. This commit attempts to resolve the conflicts between the refactor and this effort as well as massage the changes introduced here to reflect the refactor. The refactor PR can be found here: cli#10197 I'll note that there are still a few failing tests in status_test.go. I haven't had a chance to fully grok while they are failing, yet, and suspect that some insights from the original PR author may be helpful here. Full disclaimer: I haven't verified any of this is working locally yet. My primary motivation is to get these new changes working together in a manner that unblocks further iteration on this effort. * trunk: (79 commits) Enhance help docs on ext upgrade notices chore: fix some function names in comment Expand docs on cleaning extension update dir Simplifying cleanExtensionUpdateDir logic Separate logic for checking updates Capture greater detail on updaterEnabled Restore old error functionality of prSelectorForCurrentBranch Change error handling on ReadBranchConfig to respect git Exit Codes fix: add back colon that I removed fix: actually read how MaxFunc work and simplify the code fix: padded display Collapse dryrun checks in ext bin upgrade Bump github.com/mattn/go-colorable from 0.1.13 to 0.1.14 Rename test user in tests Change pr number in test Surface and handle error from ReadBranchConfig in parseCurrentBranch Directly stub headBranchConfig in Test_tryDetermineTrackingRef Refactor error handling in ReadBranchConfig to avoid panic Refine error handling of ReadBranchConfig Add test for empty BranchConfig in prSelectorForCurrentBranch ...

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

jtmcg added 3 commits January 7, 2025 13:54

Remove named returns from ReadBranchConfig and surface errors

bf6fdbd

Refactor ReadBranchConfig for test coverage of newly returned erros

f43da8e

Remove named return values from prSelectorForCurrentBranch

c83cf32

jtmcg force-pushed the jtmcg/remove-named-returns branch from af55ace to c83cf32 Compare January 7, 2025 21:59

Refactor Test_readGitBranchConfig for easier parsing

0137150

jtmcg marked this pull request as ready for review January 7, 2025 22:58

jtmcg requested a review from a team as a code owner January 7, 2025 22:58

jtmcg requested a review from andyfeller January 7, 2025 22:58

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

williammartin requested changes Jan 8, 2025

View reviewed changes

jtmcg added 5 commits January 8, 2025 10:36

Change ReadBranchConfig error message in gh pr create

fea46c0

Add missing test for RemoteURL in parseBranchConfig

527d1db

Added tests to cover prSelectorForCurrentBranch for confidence in ref…

94b2d4e

…actor

jtmcg force-pushed the jtmcg/remove-named-returns branch from de5a6af to 9f99d1f Compare January 8, 2025 23:20

Remove shadowed errors from prSelectorForCurrentBranch and cover with…

15ac566

… tests

jtmcg force-pushed the jtmcg/remove-named-returns branch from 9f99d1f to 15ac566 Compare January 8, 2025 23:21

jtmcg requested a review from williammartin January 8, 2025 23:25

jtmcg changed the title ~~Remove named return values from ReadBranchConfig and prSelectorForCurrentBranch~~ Remove naked return values from ReadBranchConfig and prSelectorForCurrentBranch Jan 8, 2025

Add test for empty BranchConfig in prSelectorForCurrentBranch

d4f7576

andyfeller reviewed Jan 9, 2025

View reviewed changes

jtmcg force-pushed the jtmcg/remove-named-returns branch from 8845239 to e1423cd Compare January 9, 2025 19:45

jtmcg added 2 commits January 9, 2025 11:45

Refactor error handling in ReadBranchConfig to avoid panic

48d45d0

Directly stub headBranchConfig in Test_tryDetermineTrackingRef

e4320cc

jtmcg added 3 commits January 9, 2025 12:46

Change pr number in test

100f9b4

Rename test user in tests

3d27487

williammartin reviewed Jan 10, 2025

View reviewed changes

git/client.go Outdated Show resolved Hide resolved

jtmcg added 2 commits January 10, 2025 09:59

Change error handling on ReadBranchConfig to respect git Exit Codes

322a43f

jtmcg requested a review from williammartin January 10, 2025 19:28

andyfeller approved these changes Jan 10, 2025

View reviewed changes

williammartin approved these changes Jan 13, 2025

View reviewed changes

jtmcg merged commit 6fe21d8 into trunk Jan 13, 2025

jtmcg deleted the jtmcg/remove-named-returns branch January 13, 2025 17:55

jtmcg mentioned this pull request Jan 13, 2025

pr view and pr status should respect @{push} where possible #9363

Closed

Remove naked return values from ReadBranchConfig and prSelectorForCurrentBranch #10197

Remove naked return values from ReadBranchConfig and prSelectorForCurrentBranch #10197

Uh oh!

Conversation

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

Uh oh!

Motivation

Changes

Testing

Uh oh!

BagToad commented Jan 7, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtmcg commented Jan 8, 2025

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment • 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!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

williammartin commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` #10197

Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` #10197

jtmcg commented Jan 7, 2025 •

edited

Loading

jtmcg commented Jan 7, 2025 •

edited

Loading

williammartin commented Jan 8, 2025 •

edited

Loading

andyfeller left a comment •

edited

Loading