Add JsonArtifact
#937
Merged
Add JsonArtifact
#937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
|
Could you throw in a |
collindutter
reviewed
Jul 3, 2024
griptape/artifacts/json_artifact.py
Outdated
|
|
||
| @property | ||
| def value(self) -> Any: | ||
| return json.loads(self.value) |
There was a problem hiding this comment.
The codecov warning here seems worth testing.
collindutter
reviewed
Jul 3, 2024
pyproject.toml
Outdated
| @@ -28,6 +28,7 @@ stringcase = "^1.2.0" | |||
| docker = "^7.1.0" | |||
| sqlalchemy = "~=1.0" | |||
| requests = "^2.32.0" | |||
| jsonalias = "^0.1.2" | |||
There was a problem hiding this comment.
I'm not sure this is even worth including since its usage is so limited. Can you just include the type in JsonArtifact?
collindutter
reviewed
Jul 3, 2024
| def test_value_type_conversion(self): | ||
| assert JsonArtifact({"foo": "bar"}).value == json.loads(json.dumps({"foo": "bar"})) | ||
| string = json.dumps({"foo": {}}) | ||
| print(string) |
| string = json.dumps({"foo": {}}) | ||
| print(string) | ||
| string = json.loads(string) | ||
| print(string) |
vachillo
force-pushed
the
json_artifact
branch
2 times, most recently
from
9782878 to
6c894d9
Compare
vachillo
force-pushed
the
json_artifact
branch
from
6c894d9 to
5a1c7b6
Compare
collindutter
requested changes
Aug 15, 2024
| @@ -58,3 +58,7 @@ A [BooleanArtifact](../../reference/griptape/artifacts/boolean_artifact.md) is u | |||
| A [GenericArtifact](../../reference/griptape/artifacts/generic_artifact.md) can be used as an escape hatch for passing any type of data around the framework. | |||
| It is generally not recommended to use this Artifact type, but it can be used in a handful of situations where no other Artifact type fits the data being passed. | |||
| See [talking to a video](../../examples/talk-to-a-video.md) for an example of using a `GenericArtifact` to pass a Gemini-specific video file. | |||
|
|
|||
| ## JsonArtifact | |||
There was a problem hiding this comment.
Header should just be Json
griptape/artifacts/json_artifact.py
Outdated
| @define | ||
| class JsonArtifact(BaseArtifact): | ||
| _obj: Any = field(metadata={"serializable": True}, alias="obj") | ||
| value: dict = field(init=False, metadata={"serializable": True}) |
griptape/artifacts/json_artifact.py
Outdated
| def __attrs_post_init__(self) -> None: | ||
| if self._obj is None: | ||
| self._obj = {} | ||
| self._json_str = json.dumps(self._obj) |
There was a problem hiding this comment.
Why do we need a field for this? Can we just dumps in to_text?
griptape/artifacts/json_artifact.py
Outdated
|
|
||
| @define | ||
| class JsonArtifact(BaseArtifact): | ||
| _obj: Any = field(metadata={"serializable": True}, alias="obj") |
There was a problem hiding this comment.
What does this field provide?
There was a problem hiding this comment.
this is just the original object being converted. different than value. consistent with json.dumps signature.
Comment on lines
1
to
25
| import json | ||
|
|
||
| import pytest | ||
|
|
||
| from griptape.artifacts import JsonArtifact, TextArtifact | ||
|
|
||
|
|
||
| class TestJsonArtifact: | ||
| def test_value_type_conversion(self): | ||
| assert JsonArtifact({"foo": "bar"}).value == json.loads(json.dumps({"foo": "bar"})) | ||
|
|
||
| def test___add__(self): | ||
| assert (JsonArtifact({"foo": "bar"}) + JsonArtifact({"value": "baz"})).value == JsonArtifact( | ||
| {"foo": "bar", "value": "baz"} | ||
| ).value | ||
|
|
||
| new = JsonArtifact({"foo": "bar"}) + JsonArtifact({"value": "baz"}) | ||
| assert new.value == {"foo": "bar", "value": "baz"} | ||
|
|
||
| assert (JsonArtifact({"foo": "bar"}) + JsonArtifact({"foo": "baz"})).value == JsonArtifact({"foo": "baz"}).value | ||
| with pytest.raises(ValueError): | ||
| JsonArtifact({"foo": "bar"}) + TextArtifact("invalid json") | ||
|
|
||
| def test_to_text(self): | ||
| assert JsonArtifact({"foo": "bar"}).to_text() == json.dumps({"foo": "bar"}) |
There was a problem hiding this comment.
Can you please add tests for all the types that Json can be? dict[str, "Json"], list["Json"], str, int, float, bool, None
collindutter
previously approved these changes
Aug 15, 2024
collindutter
approved these changes
Aug 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
closes #935
📚 Documentation preview 📚: https://griptape--937.org.readthedocs.build//937/