Skip to content
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

Fix things in the fastapi tutorial #8398

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

Conversation

diksipav
Copy link
Contributor

@diksipav diksipav commented Feb 26, 2025

07.03.2025: Even now whenever I use "gel" in HN queries to HN I get responses that there's nothing about gel, querying edgedb works. So not sure should we use gel or edgedb when searching HN.

@elprans I still didn't read the whole tutorial. I fixed the code so that it runs, and I fixed wording at places. I think it needs more work, and it can probably be updated to be shorter.

@diksipav diksipav requested a review from anbuzin February 26, 2025 11:24
Copy link

github-actions bot commented Feb 26, 2025

Docs preview deploy

❌ Docs preview deployment failed for commit a9d2c04:

https://edgedb-docs-dzi5guwbt-edgedb.vercel.app

(Last updated: Mar 7, 2025, 20:18:01 UTC)

@anbuzin
Copy link
Contributor

anbuzin commented Feb 26, 2025

QUESTION I think we dont think uv sync after installing deps, .env is created with first pkg install. UV sync should be run when u wanna install all deps at once from the pyproject.toml. Right?

uv does create a venv if you run uv add, but I believe you still need uv sync to create a lockfile.

@diksipav
Copy link
Contributor Author

hey @deepbuzin I didn't use uv sync and I have uv.lock created together with .venv, I double tested : )
Screenshot 2025-02-26 at 13 47 24

Screenshot 2025-02-26 at 13 47 41

@diksipav diksipav changed the title Fix minor things in the fastapi tutorial Fix things in the fastapi tutorial Feb 27, 2025
@diksipav diksipav force-pushed the fix-fastapi-tutorial branch 2 times, most recently from 1f5da88 to b64c823 Compare February 28, 2025 10:44
@diksipav diksipav force-pushed the fix-fastapi-tutorial branch from b64c823 to b9e0597 Compare February 28, 2025 13:04
@@ -116,19 +113,24 @@ with semantic search-based cross-chat memory.
Once the server gets up and running, we can make sure it works using FastAPI's
built-in UI at <http://127.0.0.1:8000/docs>_, or manually with ``curl``:

.. note::
To pretty-print results in this and all following cURL examples, you can use ``jq``, by piping the output to it. `jq <https://jqlang.org/>`_ is a lightweight and flexible command-line JSON processor that, among other things, formats responses with proper indentation. However, using it is optional for this tutorial.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good idea, however, we should also consider encouraging users to test endpoints using the built-in Swagger UI instead.

It runs the same exact curl commands, and it also pretty prints them. The only issue is that it's not something copy-pasteable, hence all the bash commands across the tutorial

Copy link
Contributor Author

@diksipav diksipav Mar 6, 2025

Choose a reason for hiding this comment

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

hm I am not sure how do u mean to encourage users to use swagger ui/redoc? we can mention that this is possible somewhere in the tutorial

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we could use most of the space in this note to direct people to localhost:3000 and explain that they can do those same curl requests there through the GUI. And then also mention jq for those terminal addicts that don't know what a browser is.



.. edb:split-section::

This created the ``app/sample_data/inserts.edgeql`` file, which we can now execute
using the CLI like this:
This created the ``app/sample_data/inserts.edgeql`` file, which we can execute using the CLI:
Copy link
Contributor Author

@diksipav diksipav Mar 7, 2025

Choose a reason for hiding this comment

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

I believe we can remove

    {"id": "862de904-de39-11ef-9713-4fab09220c4a"}
     {"id": "862e400c-de39-11ef-9713-2f81f2b67013"}
     {"id": "862de904-de39-11ef-9713-4fab09220c4a"}
     {"id": "862e400c-de39-11ef-9713-2f81f2b67013"}

from below.

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.

4 participants