Skip to content

Visitor generator cleanup#5770

Merged
sougou merged 4 commits intovitessio:masterfrom
systay:rewriter
Feb 3, 2020
Merged

Visitor generator cleanup#5770
sougou merged 4 commits intovitessio:masterfrom
systay:rewriter

Conversation

@systay
Copy link
Copy Markdown
Collaborator

@systay systay commented Jan 27, 2020

No description provided.

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay requested a review from dweitzman January 27, 2020 10:26
@systay systay requested a review from sougou as a code owner January 27, 2020 10:26
Copy link
Copy Markdown
Collaborator

@dweitzman dweitzman left a comment

Choose a reason for hiding this comment

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

Thanks, looks good

exit 1
fi
rm -f tmp_rewriter.go No newline at end of file
go run ./go/vt/sqlparser/visitorgen/main -compareOnly=true -input=go/vt/sqlparser/ast.go -output=go/vt/sqlparser/rewriter.go No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of (or in addition to?) a precommit hook, what about adding this to the list of tests in ./test/config.json so that when somebody forgets to regenerate the rewriter it blocks merging the commit on github?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Both, and one depending on the other so they always agree is what I would vote for

Signed-off-by: Andres Taylor <andres@planetscale.com>
Copy link
Copy Markdown
Contributor

@saifalharthi saifalharthi left a comment

Choose a reason for hiding this comment

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

LGTM

@sougou sougou merged commit 6a71f24 into vitessio:master Feb 3, 2020
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