Skip to content

feat: Add user utility#88

Closed
ramonasuncion wants to merge 1 commit into
IntelligentSandbox:mainfrom
ramonasuncion:test-review
Closed

feat: Add user utility#88
ramonasuncion wants to merge 1 commit into
IntelligentSandbox:mainfrom
ramonasuncion:test-review

Conversation

@ramonasuncion

Copy link
Copy Markdown
Member

No description provided.

@ramonasuncion

Copy link
Copy Markdown
Member Author

@snipercodeai review

@@ -0,0 +1,9 @@
def get_user(users: list, user_id: int) -> dict:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Type hinting: The users parameter is hinted as a list, but its contents are dictionaries. Consider using list[dict] for better type hinting.

@@ -0,0 +1,9 @@
def get_user(users: list, user_id: int) -> dict:
for user in users:
if user["id"] == user_id:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Potential Bug: This function will return None if no user with the given user_id is found. Consider raising a ValueError or returning an empty dictionary instead.



def get_username(users: list, user_id: int) -> str:
user = get_user(users, user_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: If get_user returns None, this line will raise a TypeError. Add a null check: if user is None: raise ValueError('User not found') or handle it in the get_user function as suggested above.


def get_username(users: list, user_id: int) -> str:
user = get_user(users, user_id)
return user["name"].upper()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Potential Bug: This line will raise a TypeError if user is None. Add a null check: if user is not None and 'name' in user: return user['name'].upper() to handle cases where the user or the 'name' key is missing.

return user


def get_username(users: list, user_id: int) -> str:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Function signature: The function get_username could be improved by adding a docstring to describe its purpose and parameters. Consider using a tool like Sphinx for documentation.

@ramonasuncion

Copy link
Copy Markdown
Member Author

Good stuff. A bit noisy on the docs but it did it.

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.

1 participant