Skip to content

Conversation

@emiliano-gandini-outeda
Copy link
Contributor

@emiliano-gandini-outeda emiliano-gandini-outeda commented Oct 12, 2025

Fixing point 8 if this Issue

  • Standardized boolean flag comparisons to use is False instead of not operators for improved code clarity

Extra:

  • Fixed JavaScript code snippet syntax in user management documentation

Please review to check if proper values (True/False/None) have been used.

@emiliano-gandini-outeda
Copy link
Contributor Author

I believe I'm done with this.

This closes #195

):
# 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:
Copy link
Collaborator

@igorbenav igorbenav Oct 13, 2025

Choose a reason for hiding this comment

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

exists returns a boolean

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:
Copy link
Collaborator

@igorbenav igorbenav Oct 13, 2025

Choose a reason for hiding this comment

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

exists returns a boolean

):
# 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:
Copy link
Collaborator

@igorbenav igorbenav Oct 13, 2025

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

):
filters = {"is_active": is_active}
if name:
if name is True:
Copy link
Collaborator

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

):
post = await crud_posts.get(db=db, id=post_id)
if not post:
if post is None:
Copy link
Collaborator

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

Copy link
Contributor

@carlosplanchon carlosplanchon Oct 14, 2025

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.

@emiliano-gandini-outeda
Copy link
Contributor Author

@igorbenav I believe I fixed all instances of those errors

@LucasQR LucasQR closed this Oct 15, 2025
@LucasQR
Copy link
Collaborator

LucasQR commented Oct 15, 2025

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

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