Added this alongside goimports dependency and Makefile to ensure proper import formatting.

Why use a makefile instead of inlining this single line like we do everywhere else in this GH workflow?

The line is

go run golang.org/x/tools/cmd/goimports -local github.com/temporalio/cli -w .

I'd prefer to not copy that as it contains a minor element of configuration (-local) that can go out of sync/get lost.

Also, I'd actually like any other instructions to also use the Makefile where possible to have a single source of truth. I believe CIs should as much as possible use scripts/Makefiles to ensure the local experience is as close as possible to the CI experience. Commands that are only run in CI have a tendency to break more easily in my experience. Having said that, I'm not planning to make any changes right now.

Why are we using the -local flag? I thought we voted against that in the server repo because most IDEs default to not using this mode.

Also don't understand why we need a Makefile for three trivial commands.

I'd prefer to not copy that as it contains a minor element of configuration (-local) that can go out of sync/get lost.

Why does it have to be in Makefile at all? Is this something we expect anything but CI to run? IMO we shouldn't pick platform favorites for our contributors (though I know we have with server, we traditionally have not with SDKs or CI). All things needed to contribute to this software should be platform independent IMO. The Makefile in this repository is only for optional things a user may want, it is not used in CI.

Alright guys; I'll roll it back.

Concur w/ @bergundy, -local seems like a very Temporal-specific kind of option. I think we should use default formatting options. Even if we did want this option, would not recommend using this PR to do it. You should separate the moving PRs from the file mutating ones as much as you can IMO.

My intent was to have consistent import style (newline vs no inline); and this PR seemed like the perfect place for it since it already touches all the inconsistent import lines. Since this is now consistent, too, it's all good now.

Is this new top-level file necessary? We already do a bad job of cluttering the top-level with platform/environment-specific things that continue to push the README below the fold (it was clearer back in https://github.com/temporalio/cli/tree/v0.12.0, thought arguably even Dockerfile should go). Can this be somewhere else? Or even better, installed and run as needed instead of depended on as part of the project?

👍 There are a couple of ways to do this with various tradeoffs. I moved the install into the Makefile target now.

Mentioned in other comment, but Makefile in this repository has been for optional things, not things that are required to be executed by CI or contributors. Makefiles are usually platform dependent and have never been used in CI or required by contributors for that reason.

Regardless, this file not being at the top level does alleviate my concerns, all good here.

I'd recommend the moving all of these files one deeper into internal, e.g. internal/temporalcli instead of just internal, but it's not a big deal.

It seems redundant to me since the path is already github.com/temporalio/cli/internal/ with temporalio/cli right there. What's the benefit?

Today we have everything under temporalcli because we need everything in one top-level package for visibility/clarity source code organization reasons and it was clearly named. However, now we have internal, it is unclear what should be put in the package. It'll just become a catch-all I fear (granted the existing one kinda was, but at least it was named).

But this is not a big deal to me, I'm ok w/ it like this.

👍 Makes sense; I'll add it.

Note, most things you read from https://go.dev/doc/modules/layout have sub-packages of internal, not in internal itself. I admit I have not really seen internal used as a package in itself. But not a big deal, even if a bit strange.

Why is this on it's own line? Is that the fmt-import added in this PR?

Yup; goimports -local will sort like this

import (
  // stdlib

  // third-party

  // local packages
)

FWIW, I am not a fan of -local. Why is goimports default unacceptable? Ideally we should work with default formatting options.

It's gone, so let's move on.