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

Refactored the Dialect to use an interface to generate the DB specific SQL. #484

Closed
wants to merge 9 commits into from

Conversation

wynan
Copy link
Contributor

@wynan wynan commented Sep 6, 2023

Plan to add SQL Server support and this seemed like the time to refactor the Dialect into a proper interface. No functional changes.

@badgerwithagun
Copy link
Member

badgerwithagun commented Sep 11, 2023

This looks sensible. However, it's quite a compatibility break. This could be avoided to a degree (if we ignore binary compatibility) by:

  1. Just change Dialect into an interface
  2. Include the original implementations as inner classes of Dialect
  3. Add public static final properties exposing instances of those inner classes to replace the enum properties

e.g.

public interface Dialect {

  static final H2 = new H2Impl();

  static final class H2Impl implements Dialect {
  }

}

I think this would clean up a lot of the "splash damage" around the rest of the codebase.

@wynan
Copy link
Contributor Author

wynan commented Sep 12, 2023

Will do. I was trying to keep Dialect an Enum and create a new concept to encapsulate the SQL differences but I'll go ahead and change Dialect to an interface.

@wynan
Copy link
Contributor Author

wynan commented Sep 12, 2023

@badgerwithagun Let me know if you see anything else you want updated or want further changes. I feel like the migration code could be refactored as well but wanted to keep this change tight and small.

@wynan
Copy link
Contributor Author

wynan commented Nov 14, 2023

@badgerwithagun Let me know if you see anything else you want updated or want further changes. I feel like the migration code could be refactored as well but wanted to keep this change tight and small.

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.

3 participants