Skip to content

Conversation

leplatrem
Copy link
Contributor

Merge into #75

Copy link
Contributor

@grahamalama grahamalama left a comment

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?


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
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
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 on lines +63 to +65
.. epigraph::

Accept its configuration through environment variables.
Copy link
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
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 <graham.beckley@gmail.com>
@leplatrem
Copy link
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
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 <graham.beckley@gmail.com>

* Mention APP_DIR for the version file

---------

Co-authored-by: grahamalama <graham.beckley@gmail.com>

* Update tox.ini

Co-authored-by: grahamalama <graham.beckley@gmail.com>

---------

Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
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