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

Cleaning up code #897

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Cleaning up code #897

wants to merge 10 commits into from

Conversation

BorzdeG
Copy link
Contributor

@BorzdeG BorzdeG commented Feb 1, 2025

discussion: #902

  • Removed duplicate code
  • Fixed the installation of the linter to the one recommended by the developer (moved to a separate PR; Fixed the installation of the linter #901 )
  • Added the ability to set a dialect not only via a string, but also via the Dialect type (moved to a separate PR: Dialect refactoring #914)
  • Added dialect type to query dialects
  • Fixed tests - sometimes they did not have time to close the database before cleaning
  • Removed package database
  • Removed StoreController
  • Refactoring the migration version entity - added zero and no version

Some of the changes were proposed in #898

2. Fixed the installation of the linter to the one recommended by the developer
@BorzdeG BorzdeG changed the title Cleaning up code from duplicates Cleaning up code Feb 2, 2025
2. Preparing to remove redundant `database.Store` and `database.StoreExtender` interfaces (partially put into pressly#898)
…oring-dialects

# Conflicts:
#	internal/dialectquery/dialectquery.go
2. Fixed tests - sometimes they did not have time to close the database before cleaning
@BorzdeG
Copy link
Contributor Author

BorzdeG commented Feb 3, 2025

@mfridman could you take a look at the draft before I finish it?

@mfridman
Copy link
Collaborator

mfridman commented Feb 3, 2025

This will be a no-go in the /v3 of goose. It's got breaking changes, and this is something we take very seriously. I.e., we cannot break users unless we cut a major /v4 version.

I am happy to accept some of these changes, like the golangci and any test fixes. But these should be broken down into smaller PRs to make reviewing easier.

For some of the bigger changes, like moving code around/renaming/etc., I'd like to understand the rationale. Is it code readability, prefactor to support new features (in a backwards-compatible way), buf fixes, etc. To avoid doing a bunch of work, it'd be helpful to get a discussion going first to see what's in/out of scope.

@BorzdeG
Copy link
Contributor Author

BorzdeG commented Feb 3, 2025

Yes, I haven't corrected the version of the package - I'll make adjustments. For the rest, I'll put them in separate PRs and start a discussion

@BorzdeG
Copy link
Contributor Author

BorzdeG commented Feb 3, 2025

started a discussion: #902

2. Removed `StoreController`
3. Refactoring the migration version entity - added zero and no version
@BorzdeG BorzdeG marked this pull request as ready for review February 3, 2025 23:03
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.

2 participants