Skip to content

Conversation

@patrick91
Copy link
Contributor

No description provided.

@patrick91 patrick91 force-pushed the 10-17-_add_support_for_reading_configuration_from_pyproject.toml branch 5 times, most recently from 404dc67 to dd1e4df Compare October 28, 2025 16:24
@patrick91 patrick91 force-pushed the 10-17-_add_support_for_reading_configuration_from_pyproject.toml branch from dd1e4df to b306a01 Compare October 28, 2025 16:28
@patrick91 patrick91 added the feature New feature or request label Oct 31, 2025
@patrick91 patrick91 marked this pull request as ready for review November 7, 2025 11:11
@tiangolo tiangolo changed the title ✨ Add support for reading configuration from pyproject.toml ✨ Add support for reading configuration from pyproject.toml Nov 9, 2025
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

This is looking great. 🐔 🎉


I saw a small thing, but also left me thinking if we should or not add the host (and port).

Comment on lines +126 to +128
host=host,
port=port,
entrypoint=entrypoint,
Copy link
Member

Choose a reason for hiding this comment

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

It seems these host and port end up coming from the default values in the function definition, so they are never read from pyproject.toml 😅

I'm also wondering if we should add the host and port to the config. 🤔

With fastapi dev it defaults to localhost (127.0.0.1), with fastapi run it defaults to all hosts (0.0.0.0). Setting the host in the config would end up taking precedence over the defaults, which could be counterintuitive, e.g. someone tries to run it somewhere in prod and because they had an override in the file, it no longer listens to all the hosts.

I think I would suggest removing host from pyproject.toml.

About port, maybe it would make sense to allow changing it, in particular if they have multiple apps and want to run them all at the same time, but we don't support that yet in this file config. But maybe it's worth keeping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants