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

*: sort all the imports #31329

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@benesch
Member

benesch commented Oct 12, 2018

I'm submitting this PR as a starting point for discussion. I've added a new flag to crlfmt, -groupimports, that performs import sorting automatically. That way adventurous folks, like myself and @jordanlewis, can opt-in for a bit and see how many spurious diffs are generated in the normal course of development.

As I mentioned on Slack, think that teaching crlfmt to do this automatically will be a net positive. crlfmt is already picky enough about function signatures that I think most everyone has their editors configured to format automatically on save, or at least have a workflow that involves running crlfmt before pushing to TeamCity. If crlfmt is good enough at sorting imports, no one will ever think about import sorting again. That's not the case in the current "try not to worry about import sorting" state of affairs. I don't nit import sorting on code reviews, but I still end up wasting time sorting imports if I add/remove an import to a file with a mis-sorted list of imports—I just can't help it.


Consistently sort all of our imports into three groups. The first group
contains stdlib imports. The second group contains third-party imports.
The third group contains imports cockroachdb imports.

This commit was automatically generated with crlfmt's new groupimports
feature. For now, to avoid burdening developers with extra CI cycles,
import sorting is not enforced by make lint. We may want to revist
this decision if the number of developers who configure their editors to
automatically group imports reaches critical mass.

Note that this import sorting is compatible with both gofmt and
goimports; that is, upon seeing this import sorting, neither gofmt nor
goimports will feel compelled to change it.

Release note: None

@benesch benesch requested review from cockroachdb/cli-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

*: sort all the imports
Consistently sort all of our imports into three groups. The first group
contains stdlib imports. The second group contains third-party imports.
The third group contains imports cockroachdb imports.

This commit was automatically generated with crlfmt's new groupimports
feature. For now, to avoid burdening developers with extra CI cycles,
import sorting is not enforced by `make lint`. We may want to revist
this decision if the number of developers who configure their editors to
automatically group imports reaches critical mass.

Note that this import sorting is compatible with both gofmt and
goimports; that is, upon seeing this import sorting, neither gofmt nor
goimports will feel compelled to change it.

Release note: None

@benesch benesch requested review from cockroachdb/admin-ui-prs as code owners Oct 12, 2018

// --vmodule flag and friends. As of 03/2017, no test in this package needs
// logging, but we want the test binary to accept the flags for uniformity
// with the other tests.
_ "github.com/cockroachdb/cockroach/pkg/util/log"

This comment has been minimized.

@benesch

benesch Oct 12, 2018

Member

Whoops. I'll have to fix this.

@benesch

benesch Oct 12, 2018

Member

Whoops. I'll have to fix this.

@bdarnell

This comment has been minimized.

Show comment
Hide comment
@bdarnell

bdarnell Oct 15, 2018

Member

Consistently sort all of our imports into three groups. The first group
contains stdlib imports. The second group contains third-party imports.
The third group contains imports cockroachdb imports.

I understand that having three groups with some imports in the "wrong" group is an eyesore. But I'd rather solve this by forbidding the third group than mandating it. Why keep cockroachdb imports separate?

Member

bdarnell commented Oct 15, 2018

Consistently sort all of our imports into three groups. The first group
contains stdlib imports. The second group contains third-party imports.
The third group contains imports cockroachdb imports.

I understand that having three groups with some imports in the "wrong" group is an eyesore. But I'd rather solve this by forbidding the third group than mandating it. Why keep cockroachdb imports separate?

@benesch

This comment has been minimized.

Show comment
Hide comment
@benesch

benesch Oct 15, 2018

Member

But I'd rather solve this by forbidding the third group than mandating it.

Sure, that works for me.

Member

benesch commented Oct 15, 2018

But I'd rather solve this by forbidding the third group than mandating it.

Sure, that works for me.

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