Add lint workflow #10

cheshire137 · 2024-10-09T21:28:47Z

Part of https://github.com/github/models/issues/330. This adds a new Actions build for running the Go linter. I also fixed outstanding things the linter flagged. I'd love to make this new build required once this PR lands, to help keep things tidy, especially once we open source the repo and get external contributors.

I tried the view, run with a prompt, run in interactive mode, and list commands locally and they seem all right. I would appreciate additional verification from reviewers that this PR doesn't break anything. 🙇🏻‍♀️

As copied from another internal repo.

.github/workflows/lint.yml

> Error return value of `io.WriteString` is not checked

Error: internal/azure_models/client.go:2:9: var-naming: don't use an underscore in package name (revive) package azure_models ^

Linter errors like: Error: internal/azuremodels/client.go:56:33: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx) httpReq, err := http.NewRequest("POST", prodInferenceURL, body) ^

Error: cmd/run/run.go:191:1: Function name: NewRunCommand, Cyclomatic Complexity: 50, Halstead Volume: 6931.94, Maintainability Index: 13 (maintidx) func NewRunCommand() *cobra.Command { ^

Working toward fixing linter issues like these: Error: cmd/run/run.go:320:5: deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop (gocritic) defer sp.Stop() ^ Error: cmd/run/run.go:327:5: deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop (gocritic) defer resp.Reader.Close() ^

cheshire137 · 2024-10-10T22:11:01Z

cmd/list/list.go

@@ -75,3 +78,13 @@ func NewListCommand() *cobra.Command {

 	return cmd
 }
+
+func filterToChatModels(models []*azuremodels.ModelSummary) []*azuremodels.ModelSummary {


Moved from the ux package because it was only used here.

cheshire137 · 2024-10-10T22:11:36Z

cmd/list/list.go

@@ -1,14 +1,15 @@
+// Package list provides a gh command to list available models.


Copilot helped with the package and function docs. cc @brannon for input on whether Copilot and I described things accurately.

cheshire137 · 2024-10-10T22:12:12Z

cmd/list/list.go

@@ -28,28 +30,29 @@ func NewListCommand() *cobra.Command {

 			token, _ := auth.TokenForHost("github.com")
 			if token == "" {
-				io.WriteString(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n")
+				util.WriteToOut(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n")


I moved io.WriteString calls to a new function that handles the returned error.

This PR is big enough already!

cheshire137 · 2024-10-10T22:15:17Z

cmd/run/run.go

@@ -176,73 +189,26 @@ func isPipe(r io.Reader) bool {
 	return false
 }

+// NewRunCommand returns a new gh command for running a model.
 func NewRunCommand() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "run [model] [prompt]",
 		Short: "Run inference with the specified model",
 		Args:  cobra.ArbitraryArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {


Lots of changes to shrink this function because the linter complained about its complexity.

cheshire137 · 2024-10-10T22:17:58Z

cmd/list/list.go


 	"github.com/cli/go-gh/v2/pkg/auth"
 	"github.com/cli/go-gh/v2/pkg/tableprinter"
 	"github.com/cli/go-gh/v2/pkg/term"
-	"github.com/github/gh-models/internal/azure_models"
+	"github.com/github/gh-models/internal/azuremodels"


The linter dislikes underscores in package names.

cheshire137 · 2024-10-10T22:18:39Z

cmd/run/run.go

@@ -439,3 +364,186 @@ func NewRunCommand() *cobra.Command {

 	return cmd
 }
+
+type runCommandHandler struct {


I made a type to bundle together various things that are used in several pieces of the run handler function, so the large function could be more easily split into multiple functions.

cheshire137 · 2024-10-10T22:20:08Z

internal/azuremodels/types.go

@@ -30,27 +36,30 @@ type ChatCompletionOptions struct {
 	TopP        *float64      `json:"top_p,omitempty"`
 }

-type ChatChoiceMessage struct {
+type chatChoiceMessage struct {


I made some types internal to this package since they weren't used outside it.

cheshire137 · 2024-10-10T22:20:21Z

internal/azuremodels/types.go

 func (m *ModelDetails) ContextLimits() string {
 	return fmt.Sprintf("up to %d input tokens and %d output tokens", m.MaxInputTokens, m.MaxOutputTokens)
 }
-
-func Ptr[T any](value T) *T {


I moved this to the util package since it felt generic.

cheshire137 · 2024-10-10T22:20:49Z

internal/ux/sorting.go

 	sort.Slice(models, func(i, j int) bool {
 		// Sort featured models first, by name
 		isFeaturedI := slices.Contains(featuredModelNames, models[i].Name)
 		isFeaturedJ := slices.Contains(featuredModelNames, models[j].Name)

 		if isFeaturedI && !isFeaturedJ {
 			return true
-		} else if !isFeaturedI && isFeaturedJ {
-			return false
-		} else {


The linter complained about using else branches when each branch returned.

cheshire137 · 2024-10-10T22:21:02Z

main.go

@@ -20,12 +21,12 @@ func main() {
 }

 func mainRun() exitCode {
-	cmd := cmd.NewRootCommand()
+	rootCmd := cmd.NewRootCommand()


The variable name cmd shadowed an import.

sgoedecke

LGTM! I've run through a bunch of commands locally on this branch and it all worked great 👍

Add lint workflow

d5f38d8

As copied from another internal repo.

cheshire137 self-assigned this Oct 9, 2024

github-advanced-security bot found potential problems Oct 9, 2024

View reviewed changes

.github/workflows/lint.yml Fixed Show fixed Hide fixed

cheshire137 added 17 commits October 9, 2024 16:43

Add some docs, handle some errors, appease the linter

0a57639

Merge branch 'main' into go-linter

2de60dc

Move WriteToOut to new util package

c0acf3f

Fix errcheck linter errors

b487253

> Error return value of `io.WriteString` is not checked

Update readme with link to gh website

4073652

More updates for the linter

7fbc049

Explicitly list permissions in linter workflow

2245c3f

Fix unhandled-error linter issues

22b2ae7

Fix paramTypeCombine linter issues

c7591b7

Rename package to azuremodels for linter

6ad6204

Error: internal/azure_models/client.go:2:9: var-naming: don't use an underscore in package name (revive) package azure_models ^

More updates for linter

2920dee

Add context parameter to pass along in requests

312c548

Linter errors like: Error: internal/azuremodels/client.go:56:33: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx) httpReq, err := http.NewRequest("POST", prodInferenceURL, body) ^

Docs updates for linter

b0832d2

Split up run func a bit for linter

88d2047

Error: cmd/run/run.go:191:1: Function name: NewRunCommand, Cyclomatic Complexity: 50, Halstead Volume: 6931.94, Maintainability Index: 13 (maintidx) func NewRunCommand() *cobra.Command { ^

More linter fixes

5baeb6b

Handle some unused writes and vars

166451c

cheshire137 commented Oct 10, 2024

View reviewed changes

Disable the last couple linters for now

24d70db

This PR is big enough already!

cheshire137 commented Oct 10, 2024

View reviewed changes

Disable the right linter

4b77a23

cheshire137 commented Oct 10, 2024

View reviewed changes

revive linter also dislikes it

a0d2e2d

cheshire137 marked this pull request as ready for review October 10, 2024 22:25

cheshire137 requested a review from a team as a code owner October 10, 2024 22:25

cheshire137 requested a review from brannon October 10, 2024 22:26

sgoedecke approved these changes Oct 10, 2024

View reviewed changes

cheshire137 merged commit f664ba4 into main Oct 11, 2024
4 checks passed

cheshire137 deleted the go-linter branch October 11, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint workflow #10

Add lint workflow #10

cheshire137 commented Oct 9, 2024 •

edited

Loading

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024 •

edited

Loading

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

cheshire137 Oct 10, 2024

sgoedecke left a comment

		@@ -1,14 +1,15 @@
		// Package list provides a gh command to list available models.

Add lint workflow #10

Add lint workflow #10

Conversation

cheshire137 commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheshire137 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgoedecke left a comment

Choose a reason for hiding this comment

cheshire137 commented Oct 9, 2024 •

edited

Loading

cheshire137 Oct 10, 2024 •

edited

Loading