Skip to content
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

*: crlfmt all code #928

Merged
merged 1 commit into from
Sep 24, 2020
Merged

*: crlfmt all code #928

merged 1 commit into from
Sep 24, 2020

Conversation

choleraehyq
Copy link
Contributor

Signed-off-by: Cholerae Hu [email protected]

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

gofmt style is dependent on the Go version you're using. Some of these changes might be reflective of that. Additionally, the Pebble developers are CockroachDB developers. In the CRDB repo we use a tool called crlfmt which extends gofmt with a few additional style changes (e.g. wrapping long function declarations). I think it would be best to run crlfmt on this code, rather than gofmt.

Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 6 files reviewed, all discussions resolved

@choleraehyq choleraehyq changed the title *: gofmt *: crlfmt all code Sep 21, 2020
@choleraehyq
Copy link
Contributor Author

@petermattis Done.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 200 files reviewed, 1 unresolved discussion (waiting on @choleraehyq and @petermattis)


compaction_picker.go, line 23 at r2 (raw file):

type compactionEnv struct {
	bytesCompacted		*uint64

What is going on here? The spaces between bytesCompacted and *uint64 were replaced with tabs. That doesn't seem right and my version of crlfmt (which I just recompiled) doesn't do this. @choleraehyq Do you have any idea how this occurred?

@choleraehyq
Copy link
Contributor Author

@petermattis I'm using the master version of crlfmt. The exact command I ran is crlfmt -w -diff=false -ignore 'vendor' ., did I miss some flag?

I run crlfmt -w compaction_picker.go and no diff printed.

@petermattis
Copy link
Collaborator

Huh. This is what I get:

~ crlfmt -diff=true compaction_picker.go | head -20
diff -u old/compaction_picker.go new/compaction_picker.go
--- old/compaction_picker.go	2020-09-22 08:00:25.000000000 -0400
+++ new/compaction_picker.go	2020-09-22 08:00:25.000000000 -0400
@@ -20,10 +20,10 @@
 const minIntraL0Count = 4

 type compactionEnv struct {
-	bytesCompacted		*uint64
-	earliestUnflushedSeqNum	uint64
-	earliestSnapshotSeqNum	uint64
-	inProgressCompactions	[]compactionInfo
+	bytesCompacted          *uint64
+	earliestUnflushedSeqNum uint64
+	earliestSnapshotSeqNum  uint64
+	inProgressCompactions   []compactionInfo
 }

 type compactionPicker interface {
@@ -41,8 +41,8 @@

I don't know what is going on here and I don't have time right now to investigate further. We can't merge this PR until it is figured out because subsequent PRs will just reverse all of this whitespace change.

@choleraehyq
Copy link
Contributor Author

@petermattis Seems it's because of github.com/cockroachdb/gostdlib/x/tools/imports. After I add fast=true, these tabs related diff disappear. Crlfmt doesn't use any dependencies management tools, so maybe we are using different versions of github.com/cockroachdb/gostdlib/x/tools/imports. I'm using the latest version of gostdlib, which is fbdd8f0abc9baea11b7bf52b6a19d5eab5e5af10.

@choleraehyq
Copy link
Contributor Author

It's a little bit hard to dig deeper into it. Because the unit tests of crlfmt itself don't pass cockroachdb/crlfmt#34 .

@petermattis
Copy link
Collaborator

It's a little bit hard to dig deeper into it. Because the unit tests of crlfmt itself don't pass cockroachdb/crlfmt#34 .

Ah, you're installing cockroachdb/crlfmt. CRDB engineers are using the crlfmt vendored into cockroachdb/cockroach. Slightly onerous, but if you check out github.com/cockroachdb/cockroach and make bin/.bootstrap you'll get the same bin/crlfmt that I use.

crlfmt -w -diff=false -ignore 'vendor' .

Signed-off-by: Cholerae Hu <[email protected]>
@choleraehyq
Copy link
Contributor Author

@petermattis Done.

Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks for figuring out that white space issue.

Reviewed 3 of 187 files at r3.
Reviewable status: 0 of 36 files reviewed, all discussions resolved (waiting on @petermattis)

@petermattis petermattis merged commit 5cfd1db into cockroachdb:master Sep 24, 2020
@choleraehyq choleraehyq deleted the gofmt branch September 25, 2020 02:41
@SWDevAngel
Copy link

@choleraehyq LAST DAY TO ORDER!
Hi Cholerae. Thank you for contributing to CockroachDB this year. As a token of our appreciation we would like to send you a gift. We tried to reach you via GitHub and on email, but if you didn’t see the message please DM me on our community Slack @lisa so I can send you a link to order your gift. (If you don’t want a gift, we also have a charitable contribution choice.) All orders must be in by December 13 (that’s tomorrow!), so please send this asap. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants