-
Notifications
You must be signed in to change notification settings - Fork 0
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
add copy and __add__ methods to ContextBuilder #16
Conversation
duplicate.context[path_str] = data | ||
else: | ||
duplicate.context[path_str]["lines"].update(data["lines"]) | ||
duplicate.context[path_str]["tags"].update(data["tags"]) |
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.
It seems you are using update
to merge dictionaries. However, update
for set
and dict
doesn't account for values like lists within the dictionaries. For duplicate.context[path_str]['tags']
, if 'tags' are lists, this could lead to undesired behavior. You may want to consider using a deeper merge function or a library function if tags contain nested structures.
if path_str not in duplicate.context: | ||
duplicate.context[path_str] = data | ||
else: | ||
duplicate.context[path_str]["lines"].update(data["lines"]) |
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.
Using update
on duplicate.context[path_str]['lines']
can be problematic if lines
contains an object that doesn't have a well-defined union operation, such as sets of custom objects. Ensure that all objects stored in lines
support a direct update
operation or convert them to a compatible type first.
comments | ||
) | ||
return duplicate | ||
|
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.
The check if not path:
here will stop execution if path
is None or empty. This is fine if this is expected to never be a valid value, but it could lead to skipped logic if a valid 'path' can sometimes be falsy (e.g., an empty string which might be a valid relative path for some systems).
MENTAT CODE REVIEW IN ACTIVE DEVELOPMENT. Only in use on mentat and internal repos. The add and copy methods you've added to ContextBuilder are critical for merging context states effectively, but ensure the merging operations consider all data types and potential edge cases to avoid logic errors. Review the use of update and handling of falsy values carefully. |
No description provided.