Skip to content

Rename module name to database name #2516

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tamaro-skaljic
Copy link

@tamaro-skaljic tamaro-skaljic commented Mar 27, 2025

Description of Changes

Fixes #2515

API and ABI breaking changes

None

Expected complexity level and risk

1

Testing

  • Make sure that there are no other locations where it needs to be renamed.

Documentation

  • search for with.*module.*name in the docs repository and replace any occurrences with with_database_name / withDatabaseName.

@tamaro-skaljic tamaro-skaljic requested a review from gefjon as a code owner March 27, 2025 18:10
@gefjon
Copy link
Contributor

gefjon commented Mar 28, 2025

Unfortunately, we can't make this change. DbConnectionBuilder::with_module_name is a part of our stable user-facing API, and so changing it would require a SemVer major version bump, which we're not willing to do at this time. We may discuss how to gradually transition to using with_database_name instead, but it's not something we can rush into.

Instead of removing the with_module_name builder method:
- add a with_database_method and
- add the deprecated-attribute to the with_module_name method.
- Also one test uses the with_module_name method instead of the with_databaseName method.
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This now LGTM

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

Retracting my approval. I was confused on PR numbers. We don't want to change the name of with_module_name at this time.

@tamaro-skaljic
Copy link
Author

Unfortunately, we can't make this change. DbConnectionBuilder::with_module_name is a part of our stable user-facing API, and so changing it would require a SemVer major version bump, which we're not willing to do at this time. We may discuss how to gradually transition to using with_database_name instead, but it's not something we can rush into.

I've reworked it in 18624e5 so that no major version bump is needed.

I would say it's better to create the new default now and update the docs and example projects so that new users use the with_database_name method of the DBConnectionBuilder instead of the with_module_name method. Then they'll have one breaking change less to migrate if with_module_name is removed. If this is merged I'll create the PRs for the docs and example pprojects too. :)

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.

Rename with_module_name to with_database_name
3 participants