sql: fix a bunch of minor spacing issues in the grammar by BramGruneir · Pull Request #25216 · cockroachdb/cockroach

BramGruneir · 2018-05-01T18:52:54Z

I was trying to do some searching and couldn't find what I was looking for due
to inconsistent spacing.

We should move towards adding a linter for the grammer.

Release note: None

cockroach-teamcity · 2018-05-01T18:53:00Z

This change is

knz · 2018-05-01T19:04:35Z

How did you do this change? Did you automate it? How?

BramGruneir · 2018-05-01T19:07:39Z

If I automated it I would have included the steps.

Finding the double, triple, quadruple spaces was easy. Just a search for and multi-line edit to fix them.

The rest was looking for : and see if things didn't line up. So that was more manually, but I needed a break from other work and just did it on autopilot while enjoying some music. Didn't take very long.

knz

This patch is a mixed bag of desirable stuff (the alignment of %type directives) and less desirable stuff. I wish we could break this up in multiple parts so we can discuss them separately.

I am also a little bit concerned that any subsequent patches on the 2.0 branch will get merge conflicts.

knz · 2018-05-01T19:14:16Z

pkg/sql/parser/sql.y


 // Note: newlines between non-terminals matter to the doc generator.

-collation_name:   unrestricted_name


This one-line change and those below don't seem justified - there's nothing to align wish, so why the different amount of spacing?

knz · 2018-05-01T19:14:49Z

pkg/sql/parser/sql.y


-%token <str>   ZONE
+%token <str> ZONE



All the changes above this point are not justified -- the code was homogeneous, there was no irregularity.

That's not entirely true.

%token <*tree.NumVal> ICONST FCONST uses only one space already.

I just don't know of any instance in which we use a double spaces for

ok let's keep your change here.

knz · 2018-05-01T19:15:10Z

pkg/sql/parser/sql.y

+%token NOT_LA WITH_LA AS_LA

 %union {
-  id             int


this struct change does not appear justiifed

This one was weird. I don't understand the huge spacing. I can revert it.

let's keep it your way.

knz · 2018-05-01T19:17:34Z

pkg/sql/parser/sql.y

  {
    $$.val = &tree.AlterTableDropColumn{
      ColumnKeyword: $2.bool(),
-      IfExists: true,


from here on to my next comment, I see where you're coming from but I'm not sure the change is properly justified. I would even argue we're making a step back: because we don't have gofmt support for goyacc files (nor editor support to auto-align struct initializers), requiring extra spaces is actually extra work (now people have to manually align things with their space bar).

What do you think?

More than half of these were already aligned as if it were using gofmt. So that's why I made the change. We should have a consistent style and we don't. I opted for the style that matches the rest of our codebase.

Manually adding the spaces ins't a big deal but without linter support, it will not be enforceable.

There IS yacc file support in a lot of editors, but our specific blend of go and the grammar broke every one that I tried.

More than half of these were already aligned as if it were using gofmt.

I don't see that's true. Can you show me?

Just went back through and did a bit of an audit of them. Most were not formatted. But there is a mix.

Take a look at the $$.val = &tree.CreateIndex{ Of the 4, 2 were formatted, 1 was not, and 1 was half formatted.

I'd still rather we format them in gofmt style to match the rest of our codebase. But I'll leave the final decision up to you. I find it more readable and I don't think the manual alignment is hard to do or enforce. But the most important thing here is to be consistent. So let me know either way.

knz

I am coming to like most of the changes. I still have reservations about the struct initializations. Please provide more information (see comment below).

knz · 2018-05-03T18:12:00Z

pkg/sql/parser/sql.y


-%token <str>   ZONE
+%token <str> ZONE



ok let's keep your change here.

knz · 2018-05-03T18:12:13Z

pkg/sql/parser/sql.y

+%token NOT_LA WITH_LA AS_LA

 %union {
-  id             int


let's keep it your way.

knz · 2018-05-03T18:13:02Z

pkg/sql/parser/sql.y

  {
    $$.val = &tree.AlterTableDropColumn{
      ColumnKeyword: $2.bool(),
-      IfExists: true,


More than half of these were already aligned as if it were using gofmt.

I don't see that's true. Can you show me?

knz · 2018-05-14T19:37:45Z

Let's skip the struct alignment for now unless you also can implement a linter for it straight away. I don't see anyone in the team willing (or able) to properly check this manually during future reviews.

The rest LGTM

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

I was trying to do some searching and couldn't find what I was looking for due to inconsistent spacing. We should move towards adding a linter for the grammer. Release note: None

BramGruneir · 2018-05-15T17:59:34Z

Done. I've created a branch with just the spacing changes for for the struct alignment, so we don't have to start from scratch if we do move in that direction.

https://github.com/BramGruneir/cockroach/tree/spacing2

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

BramGruneir · 2018-05-15T18:05:26Z

bors r+

knz · 2018-05-15T18:06:04Z

Thanks!

craig · 2018-05-15T18:21:10Z

Merge conflict (retrying...)

BramGruneir · 2018-05-15T19:20:34Z

bors r+

craig · 2018-05-15T19:34:47Z

Merge conflict (retrying...)

BramGruneir · 2018-05-15T19:42:51Z

bors r+

craig · 2018-05-15T20:57:10Z

Build failed (retrying...)

GitHub CI (Cockroach)

BramGruneir · 2018-05-15T21:26:10Z

bors r+

…

On Tue, May 15, 2018, 16:57 craig[bot] ***@***.***> wrote: Build failed (retrying...) - GitHub CI (Cockroach) <https://teamcity.cockroachdb.com/viewLog.html?buildId=659931&buildTypeId=Cockroach_UnitTests> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25216 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABihuZexSEgDaORPwKpWfZe2bj2VPy4Bks5ty0EugaJpZM4TuVnL> .

craig · 2018-05-15T21:26:13Z

Not awaiting review

@bdarnell

25216: sql: fix a bunch of minor spacing issues in the grammar r=BramGruneir a=BramGruneir I was trying to do some searching and couldn't find what I was looking for due to inconsistent spacing. We should move towards adding a linter for the grammer. Release note: None 25322: storage: proactively renew r1's lease r=a-robinson a=a-robinson This mechanism won't scale incredibly well to proactively renewing all expiration-based leases since it'd require a goroutine per lease. It should be fine if we only auto-renew r1's lease, though. Fixes #24753 Release note: None ---------------- @bdarnell, do you have any suggestions for testing this? I've verified it manually, but it's hard to properly isolate this code in a realistic way. We could ratchet the lease durations way down and wait until the lease's expiration bumps, but that creates a timing-based test that is either flaky if we use a really short lease duration or slow if we use a long one. Co-authored-by: Bram Gruneir <bram@cockroachlabs.com> Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>

craig · 2018-05-15T21:45:47Z

Build succeeded

GitHub CI (Cockroach)

BramGruneir added this to the 2.1 milestone May 1, 2018

BramGruneir requested review from a team and knz May 1, 2018 18:52

knz reviewed May 1, 2018

View reviewed changes

knz reviewed May 3, 2018

View reviewed changes

BramGruneir force-pushed the spacinmg branch from f3a6229 to 5faff5c Compare May 14, 2018 18:45

sql: fix a bunch of minor spacing issues in the grammar

59b81c7

I was trying to do some searching and couldn't find what I was looking for due to inconsistent spacing. We should move towards adding a linter for the grammer. Release note: None

BramGruneir force-pushed the spacinmg branch from 5faff5c to 59b81c7 Compare May 15, 2018 17:53

craig bot merged commit 59b81c7 into cockroachdb:master May 15, 2018

BramGruneir mentioned this pull request May 16, 2018

backport-2.0: sql: remove unneeded row definitions in the grammar #25578

Closed

BramGruneir deleted the spacinmg branch May 16, 2018 17:26


		// Note: newlines between non-terminals matter to the doc generator.

		collation_name: unrestricted_name

Conversation

BramGruneir commented May 1, 2018

Uh oh!

cockroach-teamcity commented May 1, 2018

Uh oh!

knz commented May 1, 2018

Uh oh!

BramGruneir commented May 1, 2018

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knz commented May 14, 2018

Uh oh!

BramGruneir commented May 15, 2018

Uh oh!

BramGruneir commented May 15, 2018

Uh oh!

knz commented May 15, 2018

Uh oh!

craig bot commented May 15, 2018

Merge conflict (retrying...)

Uh oh!

BramGruneir commented May 15, 2018

Uh oh!

craig bot commented May 15, 2018

Merge conflict (retrying...)

Uh oh!

BramGruneir commented May 15, 2018

Uh oh!

craig bot commented May 15, 2018

Build failed (retrying...)

Uh oh!

BramGruneir commented May 15, 2018 via email

Uh oh!

craig bot commented May 15, 2018

Uh oh!

craig bot commented May 15, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants