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

feat: expose normalizeSynonyms #721

Merged
merged 2 commits into from
Jun 4, 2022
Merged

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented May 24, 2022

No description provided.

@sio4
Copy link
Member

sio4 commented May 24, 2022

Hi @aeneasr,
Could you please explain what problem this PR solves?

@aeneasr
Copy link
Member Author

aeneasr commented May 24, 2022

This method is useful to get aliases for migrations working!

Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

IMHO, exporting many functions as the public function could make the internal code change harder. If this (keep it private) causes a solid problem, exporting the function could be a good thing but I can't imagine a solid issue here. Could you please add some more detailed issue descriptions?

pop.go Outdated Show resolved Hide resolved
@sio4 sio4 assigned sio4 and unassigned sio4 May 25, 2022
@sio4 sio4 added the enhancement New feature or request label May 25, 2022
@aeneasr
Copy link
Member Author

aeneasr commented Jun 3, 2022

@sio4 any update?

@sio4
Copy link
Member

sio4 commented Jun 3, 2022

In fact, I wanted to keep functions private if there is no solid reason from the perspective of the package pop itself since that alias logic is originally for internal purposes, and it will not be easy to change the API once it was exposed. At the same time, I think it could be fine because the function to be exposed by this PR is simple and I think there is not much chance to change its behavior.

One thing I want to fix is the name of the function. Actually, I am not a native speaker so my impression of the function name could be wrong but I don't think the following statement is smooth. (Since it will be exposed as fixed name, I think we need to consider it more carefully.)

switch NormalizeDialectSynonyms(driverName) {
...
}

cd.Dialect = NormalizeDialectSynonyms(dialect)

The function name is a verb but it does nothing, just returns a value. How about something like the below?

switch DialectForSynonym(driverName) {
...
}

cd.Dialect = NormalizedDialect(dialect)

I feel DialectForSynonym() is better (straight) but if you have another idea, I would like to hear your opinion.

@sio4 sio4 force-pushed the expose-normalizeSynonyms branch from f1fb87b to 59e3ada Compare June 3, 2022 14:50
@aeneasr
Copy link
Member Author

aeneasr commented Jun 3, 2022

DialectForSynonym sounds great :) Could also be CanonicalizeDatabaseType(name string)

@sio4
Copy link
Member

sio4 commented Jun 3, 2022

DialectForSynonym sounds great :) Could also be CanonicalizeDatabaseType(name string)

Oh, indeed!

But would be better to be CanonicalDatabaseType or CanonicalDialect as a form of "noun" name. To align with the term we used and other function names like DialectSupported, let's name it CanonicalDialect.

Will you update the PR? If your sun is already down, I can do that for you.

@sio4
Copy link
Member

sio4 commented Jun 4, 2022

Just pushed a rename commit to speed up (you already waited too long, sorry for that! :-)

Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sio4 sio4 merged commit 3d41646 into development Jun 4, 2022
@sio4 sio4 deleted the expose-normalizeSynonyms branch June 4, 2022 06:33
@sio4
Copy link
Member

sio4 commented Jun 4, 2022

Thanks for your patience @aeneasr! I merged the PR into the development branch after renaming the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants