Conversation
Codecov Report
@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 80.71% 84.59% +3.87%
==========================================
Files 119 119
Lines 5295 5302 +7
==========================================
+ Hits 4274 4485 +211
+ Misses 1021 817 -204
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
hanxiao
reviewed
Mar 15, 2022
| return cls.load_json(file, protocol=protocol, encoding=encoding, **kwargs) | ||
| from .... import Document | ||
|
|
||
| json_docs = json.loads(file) |
Member
There was a problem hiding this comment.
are your sure this maps to the type hint?
file: Union[str, bytes, bytearray, TextIO],
either this line or type hint, one of them is wrong
Member
Author
There was a problem hiding this comment.
fixed. Only str, bytes and bytearray are kept while TextIO and str are supported for load_json
numb3r3
previously requested changes
Mar 15, 2022
docarray/document/mixins/porting.py
Outdated
|
|
||
| return MessageToJson(self.to_protobuf(), **kwargs) | ||
| elif protocol == 'dynamic': | ||
| return json.dumps(self.to_dict(protocol=protocol)) |
Contributor
There was a problem hiding this comment.
pass the kwargs for json.dumps(.., **kwargs)
Member
Author
There was a problem hiding this comment.
dumping is not supported anymore
hanxiao
reviewed
Mar 16, 2022
docarray/document/mixins/porting.py
Outdated
Comment on lines
+86
to
+92
| elif protocol == 'dynamic': | ||
| warnings.warn( | ||
| f'protocol=`{protocol}` is not supported, ' | ||
| f'the result dict is a Python dynamic typing dict without any promise on the schema.' | ||
| 'The result dict is a Python dynamic typing dict without any promise on the schema.' | ||
| ) | ||
| return dataclasses.asdict(self._data) | ||
| res = dataclasses.asdict(self._data) | ||
| del res['_reference_doc'] | ||
| return res |
Member
There was a problem hiding this comment.
no need, you only need it in load/from
we do not support arbitrary/unschema-ed json dumping
hanxiao
reviewed
Mar 16, 2022
docarray/array/mixins/io/json.py
Outdated
Comment on lines
+49
to
+47
| :param protocol: `jsonschema` or `protobuf` | ||
| :param protocol: `jsonschema`, `protobuf` or `dynamic` |
hanxiao
reviewed
Mar 16, 2022
docarray/document/mixins/porting.py
Outdated
| pb_msg = DocumentProto() | ||
| json_format.ParseDict(obj, pb_msg, **kwargs) | ||
| return cls.from_protobuf(pb_msg) | ||
| elif protocol == 'dynamic': |
Member
There was a problem hiding this comment.
Suggested change
| elif protocol == 'dynamic': | |
| else: |
hanxiao
reviewed
Mar 16, 2022
docarray/document/mixins/porting.py
Outdated
Comment on lines
67
to
70
| elif protocol == 'dynamic': | ||
| return cls.from_dict(json.loads(obj), protocol=protocol) | ||
| else: | ||
| raise ValueError(f'protocol=`{protocol}` is not supported') |
Member
There was a problem hiding this comment.
Suggested change
| elif protocol == 'dynamic': | |
| return cls.from_dict(json.loads(obj), protocol=protocol) | |
| else: | |
| raise ValueError(f'protocol=`{protocol}` is not supported') | |
| else: | |
| return cls(json.loads(obj)) |
Member
Author
There was a problem hiding this comment.
done but using return cls.from_dict(json.loads(obj), protocol=protocol)
hanxiao
reviewed
Mar 16, 2022
| ``` | ||
|
|
||
| By default, it uses {ref}`JSON Schema and pydantic model<schema-gen>` for serialization, i.e. `protocol='jsonschema'`. You can switch the method to `protocol='protobuf'`, which leverages Protobuf as the JSON serialization backend. | ||
| By default, it uses {ref}`JSON Schema and pydantic model<schema-gen>` for serialization, i.e. `protocol='jsonschema'`. You can switch the method to `protocol='protobuf'`, which leverages Protobuf as the JSON serialization backend or `protocol='dynamic'` which accepts schemaless Documents. |
Member
There was a problem hiding this comment.
Suggested change
| By default, it uses {ref}`JSON Schema and pydantic model<schema-gen>` for serialization, i.e. `protocol='jsonschema'`. You can switch the method to `protocol='protobuf'`, which leverages Protobuf as the JSON serialization backend or `protocol='dynamic'` which accepts schemaless Documents. | |
| By default, it uses {ref}`JSON Schema and pydantic model<schema-gen>` for serialization, i.e. `protocol='jsonschema'`. You can switch the method to `protocol='protobuf'`, which leverages Protobuf as the JSON serialization backend. | |
| To load an arbitrary JSON file, please set `protocol=None`. But as it is "arbitrary", you should not expect it can be succesfully loaded. DocArray tries its best reasonable effort by first load this JSON into `dict` and then load it via `Document(dict)`. |
hanxiao
reviewed
Mar 16, 2022
hanxiao
reviewed
Mar 16, 2022
|
📝 Docs are deployed on https://ft-fix-json-save-load--jina-docs.netlify.app 🎉 |
hanxiao
approved these changes
Mar 16, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.