Skip to content

📝 Update code examples in docs for body, replace name create_item with update_item when appropriate#5913

Merged
tiangolo merged 2 commits intofastapi:masterfrom
OttoAndrey:master
Jan 9, 2024
Merged

📝 Update code examples in docs for body, replace name create_item with update_item when appropriate#5913
tiangolo merged 2 commits intofastapi:masterfrom
OttoAndrey:master

Conversation

@OttoAndrey
Copy link
Contributor

@OttoAndrey OttoAndrey commented Jan 22, 2023

Hi! I did some improvements for code exmples;

  1. Renamed function because it is put endpoint and logicaly it should be update not create;
  2. Removed redundant if statement because q param in those examples are required. Or I can add None to annotations;

@github-actions
Copy link
Contributor

📝 Docs preview for commit c9d3b22 at: https://63ccee9f0fb8da5c9c02fac2--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit dc6946f at: https://63ccf0f8cf78ab6745a882e4--fastapi.netlify.app

Copy link

@r0b2g1t r0b2g1t left a comment

Choose a reason for hiding this comment

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

Looks good for me.

Copy link

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

Tests pass.

The changes listed as "1" are good changes.

I understand the suggestion for "2", however I suggest not making those changes because that code is repeated in multiple places throughout the tutorials. As I understand the tutorials, they build on each other somewhat, and they focus on specific items in each section. It seems to me that making these changes in a few places adds inconsistencies that the learner has to investigate as they change. It may be more useful to keep it consistent even if the example isn't optimal?

@OttoAndrey
Copy link
Contributor Author

OttoAndrey commented Jan 23, 2023

I understand the suggestion for "2", however I suggest not making those changes because that code is repeated in multiple places throughout the tutorials. As I understand the tutorials, they build on each other somewhat, and they focus on specific items in each section. It seems to me that making these changes in a few places adds inconsistencies that the learner has to investigate as they change. It may be more useful to keep it consistent even if the example isn't optimal?

@Ryandaydev First time when learner see this q parameter in this article https://fastapi.tiangolo.com/tutorial/query-params/#optional-parameters . And here it looks logical because q is optional and code example has if condition for that parameter. For consisten I suggest add None for q param inside annotations. After this if statement make sense. And it will look like previous examples with q param:

@app.get("/items/{item_id}")
async def read_items(q: str | None, item_id: int = Path(title="The ID of the item to get")):
    results = {"item_id": item_id}
    if q:
        results.update({"q": q})
        return results

Copy link

@Ryandaydev Ryandaydev left a comment

Choose a reason for hiding this comment

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

I understand the reasoning on this change. Also, the tests still pass.

@tiangolo tiangolo changed the title Update code examples in docs 📝 Update code examples in docs Mar 4, 2023
@tiangolo tiangolo added the docs Documentation about how to use FastAPI label Mar 4, 2023
@github-actions
Copy link
Contributor

📝 Docs preview for commit 2bfa458 at: https://64697a4de93dbc10f72678bd--fastapi.netlify.app

@OttoAndrey
Copy link
Contributor Author

Ok. I removed controversial changes and left only renamed functions

@tiangolo tiangolo changed the title 📝 Update code examples in docs 📝 Update code examples in docs for body, replace name create_item with update_item when appropriate Jan 9, 2024
@tiangolo
Copy link
Member

tiangolo commented Jan 9, 2024

Nice, makes sense, thank you @OttoAndrey! 🍰

And thanks everyone for the input! ☕

@tiangolo tiangolo enabled auto-merge (squash) January 9, 2024 14:24
@tiangolo tiangolo merged commit e9ffa20 into fastapi:master Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation about how to use FastAPI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants