Skip to content

Use a single source of truth for supported Dialects to prevent drift#1

Open
globalchubby wants to merge 4 commits intofix/917from
fix/917-2
Open

Use a single source of truth for supported Dialects to prevent drift#1
globalchubby wants to merge 4 commits intofix/917from
fix/917-2

Conversation

@globalchubby
Copy link
Owner

@globalchubby globalchubby commented Dec 28, 2025

This PR adds dialects.registry to prevent drift between database.NewStore's lookup map and database.ParseDialect. This registry lives in internal/dialects/registry.go. The downside is that we're now maintaining the canonical list of Dialect values in three packages (goose, database, and dialects) instead of the current two, but I thought it resulted in better encapsulation to keep the registry as an internal module instead of polluting database.

Because we have to support both string and Dialect, unless we build an additional index, one of dialects.Parse or dialects.Querier will have a suboptimal iteration over the registry. I felt that creating an additional index was overkill, so I chose to pay the penalty in ParseDialect.

I'm guessing clarity trumps performance in these code paths though, and if it's clearer to have a registry with string alias keys instead of Dialect keys, I'm open to implementing this PR that way as well.

This PR also introduces a slight improvement: dialect.Querier instances are created per dialect in database.NewStore, instead of eagerly all-at-once.

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.

1 participant