-
Notifications
You must be signed in to change notification settings - Fork 238
Issue documentation improvements #201
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
Issue documentation improvements #201
Conversation
|
I believe I'm done with this. This closes #195 |
docs/user-guide/api/endpoints.md
Outdated
| ): | ||
| # Check if user exists | ||
| if not await crud_users.exists(db=db, id=user_id): | ||
| if await crud_users.exists(db=db, id=user_id) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists returns a boolean
docs/user-guide/api/endpoints.md
Outdated
| db: Annotated[AsyncSession, Depends(async_get_db)] | ||
| ): | ||
| if not await crud_users.exists(db=db, id=user_id): | ||
| if await crud_users.exists(db=db, id=user_id) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists returns a boolean
docs/user-guide/api/endpoints.md
Outdated
| ): | ||
| # Check if user already exists | ||
| if await crud_users.exists(db=db, email=user_data.email): | ||
| if await crud_users.exists(db=db, email=user_data.email) is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://peps.python.org/pep-0008/#programming-recommendations
if x is preferred to if x == True and if x is True
docs/user-guide/api/endpoints.md
Outdated
| ): | ||
| filters = {"is_active": is_active} | ||
| if name: | ||
| if name is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work only if name is a boolean, which it isn't. It's breaking logic
docs/user-guide/api/exceptions.md
Outdated
| ): | ||
| post = await crud_posts.get(db=db, id=post_id) | ||
| if not post: | ||
| if post is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with the other one because if we change it in the future to return something else, None will not work. Maybe some people would like to comment on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not should be kept in case falsy values are expected in the future.
However, a warning notice should be added to the website (in exceptions.md) to clearly inform users about this behavior, specifically, to make them aware of possible future changes in fast-crud regarding this.
|
@igorbenav I believe I fixed all instances of those errors |
|
Hey @emiliano-gandini-outeda, I just reviewed this PR and I'll have to close it since the boolean comparisons don't fit the standard of the code. Thanks for the contribution |
Fixing point 8 if this Issue
is Falseinstead ofnotoperators for improved code clarityExtra:
Please review to check if proper values (True/False/None) have been used.