Skip to content

FastAPI integration docs - #95

Merged
leplatrem merged 3 commits into
fastapi-integrationfrom
fastapi-integration-docs
Feb 20, 2024
Merged

FastAPI integration docs#95
leplatrem merged 3 commits into
fastapi-integrationfrom
fastapi-integration-docs

Conversation

@leplatrem

Copy link
Copy Markdown
Contributor

Merge into #75

@grahamalama grahamalama left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few inline comments, and this more higher-level thought:

Some of the documentation we've added feels like documentation of FastAPI vs how to integrate Dockerflow's FastAPI extension into a FastAPI app.

Can we link to FastAPI documentation where appropriate and focus this documentation on aspects that are Dockerflow-specific?

Comment thread docs/fastapi.rst Outdated
Comment thread docs/fastapi.rst

To facilitate this python-dockerflow comes with a FastAPI view to read the
file under path the parent directory of the app root. See the
:class:`FastAPI API docs <~fastapi.FastAPI>` for more information about the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be a link somewhere? If so, looks like it didn't link properly.
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apparently, we have the same behaviour on Flask docs https://python-dockerflow.readthedocs.io/en/main/flask.html#versions

Comment thread docs/fastapi.rst
Comment thread docs/fastapi.rst
Comment on lines +63 to +65
.. epigraph::

Accept its configuration through environment variables.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does epigraph suggest that this should have more formatting that just an indentation?
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is like a note... I'd say that it just has some indentation

Co-authored-by: grahamalama <[email protected]>
@leplatrem

Copy link
Copy Markdown
Contributor Author

Some of the documentation we've added feels like documentation of FastAPI vs how to integrate Dockerflow's FastAPI extension into a FastAPI app.

This is true about all the rest of the documentation I believe, I aligned it with other integrations actually.

Can we link to FastAPI documentation where appropriate and focus this documentation on aspects that are Dockerflow-specific?

Will see what I can do...

@grahamalama

Copy link
Copy Markdown
Contributor

This is true about all the rest of the documentation I believe, I aligned it with other integrations actually.

Ahh okay. Well then, I think internal consistency outweighs that feedback I gave. Maybe look for places were we're missing integration docs, but if that spills into FastAPI documentation, so be it.

@leplatrem
leplatrem merged commit 0ec8876 into fastapi-integration Feb 20, 2024
@leplatrem
leplatrem deleted the fastapi-integration-docs branch February 20, 2024 16:39
leplatrem added a commit that referenced this pull request Feb 21, 2024
* Add tox config for FastAPI 100

* Add dockerflow router to package root

* Add request summary middleware

* Add version endpoint and tests

* Add initial heartbeat route, check mechanism, and tests

* Refactor package structure, names of things

- Define "router" in package init, register view functions defined in views
  module
- Add test for check with custom name
- use better name for check operations

* Leverage centralized checks (#85)

* Do not test on Python 3.7

* Add 'taskName' to excluded fields

* Test with python 3.12

* Improve test coverage of FastAPI

* Blackify

* FastAPI integration docs (#95)

* Add FastAPI documentation

* Update docs/fastapi.rst

Co-authored-by: grahamalama <[email protected]>

* Mention APP_DIR for the version file

---------

Co-authored-by: grahamalama <[email protected]>

* Update tox.ini

Co-authored-by: grahamalama <[email protected]>

---------

Co-authored-by: Mathieu Leplatre <[email protected]>
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