Skip to content

Conversation

@rodionovv
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Oct 30, 2024

Coverage Status

coverage: 96.87% (+0.03%) from 96.84%
when pulling 4b59c4a on rodionovv:flavor_getter
into be6fb8b on huandu:master.

Copy link
Owner

@huandu huandu left a comment

Choose a reason for hiding this comment

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

Besides builders updated in this PR, we can also add Flavor() Flavor in Build interface and implement this new method in compiledBuilder and flavoredBuilder in ./builder.go.

createtable.go Outdated
return
}

func (ctb *CreateTableBuilder) GetFlavor() Flavor {
Copy link
Owner

Choose a reason for hiding this comment

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

The method name should be Flavor rather than GetFlavor according to Effective Go. And don't forget to add comment to describe this method.

Copy link
Author

Choose a reason for hiding this comment

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

Done

func TestCreateTableGetFlavor(t *testing.T) {
a := assert.New(t)
ctb := newCreateTableBuilder()
postgresFlavor := PostgreSQL
Copy link
Owner

Choose a reason for hiding this comment

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

It's not necessary to assign a const to a local variable. Doing this will make the test code less readable. It's recommended to use const in assertions directly.

@huandu
Copy link
Owner

huandu commented Oct 31, 2024

Thanks for your contribution! I sent you several review comments. Feel free to discuss with me if you have any thought.

Copy link
Owner

@huandu huandu left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@huandu huandu merged commit 134c901 into huandu:master Nov 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants