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

sql: Refactoring structured.proto. Renaming ColumnType Kind enum to SemanticType. #16606

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

m-schneider
Copy link
Contributor

@m-schneider m-schneider commented Jun 19, 2017

The current use of the the word kind to denote type, in the ColunmType message
is confusing. The upcoming changes to intoduce new enums to seperate semantic
types from visible types will only make this worse.

Closes #10705

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Jun 19, 2017

Oh my. A dream come true. @arjunravinarayan come check this out!

Masha:

  • please update the existing RFCs that mention the term "kind" to be consistent with the new naming ( can see two at least)
  • please also rename the field parser.DOid.kind
  • extend your commit message (and possibly the PR description) with a rationale for this change. You can take inspiration from our discussion on the other PR.

Reviewed 29 of 29 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@rjnn
Copy link
Contributor

rjnn commented Jun 19, 2017

Wonderful! Will give a thorough review shortly, but a quick note that "Closes #10705" would be useful in the commit message.

@rjnn
Copy link
Contributor

rjnn commented Jun 19, 2017

@knz, what is the "other PR"?

@m-schneider
Copy link
Contributor Author

Raphael:


Comments from Reviewable

@m-schneider
Copy link
Contributor Author

Arjun, added the issue to the commit message.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Jun 19, 2017

@arjunravinarayan the renaming discussion started in #16548 (comment)

@knz
Copy link
Contributor

knz commented Jun 19, 2017

@m-schneider you need to write literally "Fixes #10705." or "Closes #10705." on a line by itself in either the PR description or the commit message for Github to pick it up when you'll merge this.

@knz
Copy link
Contributor

knz commented Jun 19, 2017

:lgtm:


Reviewed 6 of 6 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@rjnn
Copy link
Contributor

rjnn commented Jun 20, 2017

:lgtm:


Reviewed 28 of 29 files at r1, 6 of 6 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

…emanticType.

The current use of the the word kind to denote type, in the ColunmType message
is confusing. The upcoming changes to intoduce new enums to seperate semantic
types from visible types will only make this worse.

Closes cockroachdb#10705
@m-schneider
Copy link
Contributor Author

@knz, didn't know that thanks for the tip!
Done.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

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