Skip to content

feat: expose classmethod to build tortoise config#2162

Open
waketzheng wants to merge 6 commits intotortoise:developfrom
waketzheng:feat-typed-config
Open

feat: expose classmethod to build tortoise config#2162
waketzheng wants to merge 6 commits intotortoise:developfrom
waketzheng:feat-typed-config

Conversation

@waketzheng
Copy link
Copy Markdown
Contributor

Description

  1. The init function of tortoise.context.Context is too long
  2. Add a new classmethod to TortoiseConfig which is used for sharing config between contexts

Motivation and Context

Base on #2159

This function has too many lines; shorten it.

How Has This Been Tested?

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 31, 2026

Merging this PR will not alter performance

✅ 24 untouched benchmarks


Comparing waketzheng:feat-typed-config (0cf3b41) with develop (4874995)

Open in CodSpeed

@seladb
Copy link
Copy Markdown
Contributor

seladb commented Apr 5, 2026

@waketzheng this PR has many changes that are not related to the PR title and description. Maybe you should break them into multiple PRs?

abondar
abondar previously approved these changes Apr 5, 2026
@seladb
Copy link
Copy Markdown
Contributor

seladb commented Apr 5, 2026

@waketzheng this PR still seems to have unrelated changes. We have this PR to add pre-commit, and this PR for Starlette v1 support. These changes can probably be removed from this PR

@waketzheng
Copy link
Copy Markdown
Contributor Author

@waketzheng this PR still seems to have unrelated changes. We have this PR to add pre-commit, and this PR for Starlette v1 support. These changes can probably be removed from this PR

@seladb No need to do that. I expected this PR to be reviewed after the previous one merged, so I explicitly mentioned that it’s based on #2159. Actually, I refactored the code once when fixing mypy complaints after Starlette upgraded, and then separated it into three PRs.

@seladb
Copy link
Copy Markdown
Contributor

seladb commented Apr 6, 2026

@waketzheng this PR still seems to have unrelated changes. We have this PR to add pre-commit, and this PR for Starlette v1 support. These changes can probably be removed from this PR

@seladb No need to do that. I expected this PR to be reviewed after the previous one merged, so I explicitly mentioned that it’s based on #2159. Actually, I refactored the code once when fixing mypy complaints after Starlette upgraded, and then separated it into three PRs.

@waketzheng is there a reason to open these changes as stacked PRs? It looks like these are separate changes that can each have their own PR based on develop 🤔

@waketzheng
Copy link
Copy Markdown
Contributor Author

  1. Using stacked PRs avoids merge conflicts
  2. I can prek run --all-files to check this PR (prek is a reimagined version of pre-commit, built in Rust.)
  3. You can select the latest commit (instead of all commits) to compare the code between this PR and the previous one.
image image

@seladb
Copy link
Copy Markdown
Contributor

seladb commented Apr 6, 2026

  1. Using stacked PRs avoids merge conflicts

Yes, if the changes in the stacked PRs depend on each other, otherwise there shouldn't be merge conflicts. Here it looks like the actual commit related to this PR doesn't depend on the changes in the other PR. What am I missing here? 🤔

@waketzheng
Copy link
Copy Markdown
Contributor Author

Yes, if the changes in the stacked PRs depend on each other, otherwise there shouldn't be merge conflicts. Here it looks like the actual commit related to this PR doesn't depend on the changes in the other PR. What am I missing here? 🤔

You forgot that I need the pre-commit config to enforce code style.

@seladb
Copy link
Copy Markdown
Contributor

seladb commented Apr 7, 2026

You forgot that I need the pre-commit config to enforce code style.

I'm not sure why pre-commit is needed for this PR, but anyway - the pre-commit PR was merged so this PR is simpler now 🙂

@waketzheng waketzheng requested a review from seladb April 9, 2026 17:09
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