New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: move RowFetcher and co to a new package, row #31292

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
6 participants
@jordanlewis
Member

jordanlewis commented Oct 12, 2018

This was purely mechanical renaming. The purpose is to lessen the width
of the sqlbase package, which is imported in many places.

The new package now depends on sqlbase, but far fewer packages consume
it than sqlbase.

Major changes:

RowFetcher -> row.Fetcher
RowUpdater -> row.Updater
RowDeleter -> row.Deleter

FK and cascade stuff -> row package

The only unfortunate thing here is that the FK enums all begin with
row now (e.g. row.CheckDeletes). It would be better if those got a
different package name, like fk, but I'll leave that refactor for
another time.

Somewhat related to #30001.

cc @dt, @benesch

Release note: None

@jordanlewis jordanlewis requested review from knz and BramGruneir Oct 12, 2018

@jordanlewis jordanlewis requested review from cockroachdb/core-prs as code owners Oct 12, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 12, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 12, 2018

This change is Reviewable

@jordanlewis jordanlewis requested a review from mjibson Oct 12, 2018

@knz

knz approved these changes Oct 12, 2018

:lgtm: woo yay to modularity!

minor nits below

Reviewed 50 of 50 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/distsqlrun/tablereader.go, line 20 at r1 (raw file):

	"context"

	"github.com/cockroachdb/cockroach/pkg/sql/row"

put the crdb imports below


pkg/sql/logictest/logic.go, line 41 at r1 (raw file):

	"unicode/utf8"

	"github.com/cockroachdb/cockroach/pkg/sql/row"

pull below


pkg/sql/row/cascader.go, line 30 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/util/log"
)

you could have said type ID = sqlbase.ID somewhere in this package and simplified the diff.


pkg/sql/row/fetcher.go, line 23 at r1 (raw file):

	"strings"

	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"

pull below


pkg/sql/row/fetcher_mvcc_test.go, line 23 at r1 (raw file):

	"testing"

	"github.com/cockroachdb/cockroach/pkg/sql/row"

ditto


pkg/sql/row/fetcher_test.go, line 23 at r1 (raw file):

	"testing"

	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"

ditto


pkg/sql/row/fk_test.go, line 25 at r1 (raw file):

	"testing"

	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"

ditto

@jordanlewis

This comment has been minimized.

Show comment
Hide comment
@jordanlewis

jordanlewis Oct 12, 2018

Member

TFTR! I'm going to fix these import nits, but I'd like to say that it's ridiculous that reviewers have to search for them. Why do we care? More importantly, why doesn't crlfmt do it? :)

Member

jordanlewis commented Oct 12, 2018

TFTR! I'm going to fix these import nits, but I'd like to say that it's ridiculous that reviewers have to search for them. Why do we care? More importantly, why doesn't crlfmt do it? :)

@dt

This comment has been minimized.

Show comment
Hide comment
@dt

dt Oct 12, 2018

Member

I think it was @bdarnell a couple years ago who put it "if it is good enough for goimports, it is good enough for us".

EDIT: found #6096 (comment)

Member

dt commented Oct 12, 2018

I think it was @bdarnell a couple years ago who put it "if it is good enough for goimports, it is good enough for us".

EDIT: found #6096 (comment)

@dt

dt approved these changes Oct 12, 2018

*: move RowFetcher and co to a new package, row
This was purely mechanical renaming. The purpose is to lessen the width
of the sqlbase package, which is imported in many places.

The new package now depends on sqlbase, but far fewer packages consume
it than sqlbase.

Major changes:

RowFetcher -> row.Fetcher
RowUpdater -> row.Updater
RowDeleter -> row.Deleter

FK and cascade stuff -> row package

The only unfortunate thing here is that the FK enums all begin with
`row` now (e.g. `row.CheckDeletes`). It would be better if those got a
different package name, like `fk`, but I'll leave that refactor for
another time.

Release note: None
@jordanlewis

This comment has been minimized.

Show comment
Hide comment
@jordanlewis

jordanlewis Oct 12, 2018

Member

TFTRs!

bors r+

Member

jordanlewis commented Oct 12, 2018

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Oct 12, 2018

Merge #31292
31292: *: move RowFetcher and co to a new package, row r=jordanlewis a=jordanlewis

This was purely mechanical renaming. The purpose is to lessen the width
of the sqlbase package, which is imported in many places.

The new package now depends on sqlbase, but far fewer packages consume
it than sqlbase.

Major changes:

RowFetcher -> row.Fetcher
RowUpdater -> row.Updater
RowDeleter -> row.Deleter

FK and cascade stuff -> row package

The only unfortunate thing here is that the FK enums all begin with
`row` now (e.g. `row.CheckDeletes`). It would be better if those got a
different package name, like `fk`, but I'll leave that refactor for
another time.

Somewhat related to #30001.

cc @dt, @benesch 

Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Oct 12, 2018

Member

Wow, this looks much easier than what I was trying to do. Thanks Jordan!

Member

benesch commented Oct 12, 2018

Wow, this looks much easier than what I was trying to do. Thanks Jordan!

@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 12, 2018

Build succeeded

@craig craig bot merged commit bcc52bc into cockroachdb:master Oct 12, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@danhhz

This comment has been minimized.

Show comment
Hide comment
@danhhz

danhhz Oct 12, 2018

Contributor

I missed the window, but :lgtm: anyway. Nice cleanup.

Ditto dt and ben (and radu's thumbs up). I've started opting out when reviewers nit the import block

Contributor

danhhz commented Oct 12, 2018

I missed the window, but :lgtm: anyway. Nice cleanup.

Ditto dt and ben (and radu's thumbs up). I've started opting out when reviewers nit the import block

@jordanlewis jordanlewis deleted the jordanlewis:row-package branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment