-
Notifications
You must be signed in to change notification settings - Fork 233
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
[CALCITE-6034] Add isAutoIncrement and isGenerated arguments to MetaColumn constructor #229
Conversation
nit: update commit message and PR description to be imperative (adds -> add) |
0c9d0b1
to
cd3db52
Compare
|
||
@Deprecated // to be removed before 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is the constructor that was failing when building Calcite?
Integer charOctetLength, | ||
int ordinalPosition, | ||
String isNullable, | ||
String isAutoincrement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is auto-increment one word? Do you camelCase hyphenated words? Interesting..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/questions/44170873/camel-casing-hyphenated-english-words-e-g-re-render#:~:text=1%20Answer&text=In%20camel%2Dcase%2C%20individual%20trailing,a%20single%20word%20would%20normally. this SO user makes a compelling case, sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess that's the convention! I see the same in Calcite so probably won't change it here.
|
||
/** Returns a copy of this MetaColumn, overriding the value of {@code isAutoincrement}. */ | ||
@SuppressWarnings("unused") // called from Calcite | ||
public MetaColumn withIsAutoIncrement(String isAutoincrement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make it consistent either do AutoIncremeent or Autoincrement everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, I thought about this for too long. I'll change it for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question and comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, if you squash your commits I can merge : )
ddd6fce
to
33f8aaf
Compare
No description provided.