Skip to content
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

Context builder methods #17

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Context builder methods #17

merged 2 commits into from
Apr 25, 2024

Conversation

granawkins
Copy link
Member

No description provided.

@@ -7,7 +7,7 @@ packages=["ragdaemon"]

[project]
name = "ragdaemon"
version = "0.2.3"
version = "0.2.5"
Copy link

Choose a reason for hiding this comment

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

Ensure the version update is intentional and follows semantic versioning correctly as it affects package deployment and dependency resolution.

@@ -1 +1 @@
__version__ = "0.2.3"
__version__ = "0.2.5"
Copy link

Choose a reason for hiding this comment

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

It's good to see consistency in the version update across the package definition in pyproject.toml and the module's __init__.py. Make sure all references to the version number are updated to avoid inconsistencies.

@granawkins granawkins merged commit 9de5bcb into main Apr 25, 2024
3 checks passed
return duplicate

def __add__(self, other: ContextBuilder) -> ContextBuilder:
duplicate = self.copy()
Copy link

Choose a reason for hiding this comment

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

Consider implementing merging of 'document' fields when combining two ContextBuilder instances if documents differ or need merging depending on context logic.

@@ -1,3 +1,6 @@
from __future__ import annotations

from copy import deepcopy
Copy link

Choose a reason for hiding this comment

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

Imports should be organized and grouped according to PEP 8 standards. from __future__ import annotations should be the first import, followed by standard library imports, and then third-party imports such as pathlib and typing imports.

comments
)
return duplicate

Copy link

Choose a reason for hiding this comment

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

Be cautious when splitting lines and accessing them by index, as this line assumes there is at least one line in the document. Ideally, there should be a check to ensure the document isn't empty to prevent index errors.


context = daemon.get_context("")
context.add_ref("src/interface.py:3-5")

Copy link

Choose a reason for hiding this comment

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

The line range set to [3-5] seems to overlap with subsequent tests modifying the same area. Ensure tests have independent conditions or reset the state appropriately to avoid interdependencies between tests.

@mentatai
Copy link

mentatai bot commented Apr 25, 2024

MENTAT CODE REVIEW IN ACTIVE DEVELOPMENT. Only in use on mentat and internal repos.
Please Reply with feedback.

Overall, the pull request introduces coherent version updates and extends functionality in ContextBuilder class. It would be even better to ensure robust handling of merging contexts and adding tests that validate non-overlapping conditions for better reliability. Watching out for potential mutable default arguments in methods and improving the documentation and import ordering as per PEP guidelines could help maintain the code quality.

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