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

Flavor getters #177

Merged
merged 2 commits into from
Nov 1, 2024
Merged

Flavor getters #177

merged 2 commits into from
Nov 1, 2024

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
@@ -158,6 +158,10 @@ func (ctb *CreateTableBuilder) SetFlavor(flavor Flavor) (old Flavor) {
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