Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

chore: Move codenav types to lower-level package#64141

Merged
varungandhi-src merged 3 commits into
mainfrom
vg/move-types
Jul 30, 2024
Merged

chore: Move codenav types to lower-level package#64141
varungandhi-src merged 3 commits into
mainfrom
vg/move-types

Conversation

@varungandhi-src

Copy link
Copy Markdown
Contributor

For https://github.com/sourcegraph/sourcegraph/pull/64126, I need to be able to
access some of these types in the codenav package directly, so move these
to avoid an import cycle: codenav -> resolvers -> codenav.

Test plan

Covered by existing tests

@cla-bot cla-bot Bot added the cla-signed label Jul 30, 2024
@github-actions github-actions Bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jul 30, 2024

@kritzcreek kritzcreek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just nits

Comment on lines 10 to 12
"golang.org/x/exp/rand"

"github.com/stretchr/testify/require"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"golang.org/x/exp/rand"
"github.com/stretchr/testify/require"
"golang.org/x/exp/rand"
"github.com/stretchr/testify/require"

LocationOffset int `json:"locationOffset"`
}

type UsagesForSymbolResolvedArgs struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Where possible I'd prefer declaring types next to their primary usage. Could this type be declared in service.go next to the "PreciseUsages" function that accepts it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a fair number of types here, and service.go/service_new.go primarily have the logic for the service methods themselves (whereas this file houses the related type definitions), so I'd rather keep the types in this file following the existing code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a similar pattern in codenav/shared/types.go and uploads/shared/types.go where the main types involved in parameters/results and their methods are defined in a separate types.go file

@varungandhi-src varungandhi-src merged commit 3f0a852 into main Jul 30, 2024
@varungandhi-src varungandhi-src deleted the vg/move-types branch July 30, 2024 04:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants