fix JSON bug with data reading #691
Merged
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
JGSweets
added
Bug
JGSweets
requested review from
ksneab7,
taylorfturner,
micdavis and
tyfarnan
as code owners
JGSweets
commented
Oct 14, 2022
| @@ -1,6 +1,6 @@ | |||
| """Contains class for saving and loading spreadsheet data.""" | |||
| from io import BytesIO, StringIO | |||
| from typing import Any, Dict, List, Optional, Union, cast | |||
| from typing import Any, Dict, List, Optional, Union | |||
There was a problem hiding this comment.
the proper way in the other func for a boolean which checks type is to return a TypeGuard
There was a problem hiding this comment.
no need to cast now
JGSweets
commented
Oct 14, 2022
| @@ -92,18 +92,12 @@ def is_match( | |||
|
|
|||
| # get current position of stream | |||
| if data_utils.is_stream_buffer(file_path): | |||
| file_path = cast( | |||
There was a problem hiding this comment.
static typing, not functional change, removed bc of TypeGuard
JGSweets
commented
Oct 14, 2022
| starting_location = file_path.tell() | ||
|
|
||
| is_valid_avro = fastavro.is_avro(file_path) | ||
|
|
||
| # return to original position in stream | ||
| if data_utils.is_stream_buffer(file_path): | ||
| file_path = cast( |
There was a problem hiding this comment.
static typing, not functional change, removed bc of TypeGuard
JGSweets
commented
Oct 14, 2022
|
|
||
| def is_stream_buffer(filepath_or_buffer: Any) -> bool: | ||
|
|
||
| def is_stream_buffer(filepath_or_buffer: Any) -> TypeGuard[Union[StringIO, BytesIO]]: |
There was a problem hiding this comment.
fix to use TypeGuard
JGSweets
commented
Oct 14, 2022
Comment on lines
-30
to
+35
| open_method: str ="r", | ||
| encoding: Optional[str]=None, | ||
| seek_offset: Optional[int]=None, | ||
| seek_whence: int=0, | ||
| open_method: str = "r", | ||
| encoding: Optional[str] = None, | ||
| seek_offset: Optional[int] = None, | ||
| seek_whence: int = 0, |
JGSweets
commented
Oct 14, 2022
Comment on lines
-54
to
+58
| self.original_type: Union[Type[str], Type[StringIO], Type[BytesIO], Type[IO]] = type(filepath_or_buffer) | ||
| self.original_type: Union[ | ||
| Type[str], Type[StringIO], Type[BytesIO], Type[IO] | ||
| ] = type(filepath_or_buffer) |
JGSweets
commented
Oct 14, 2022
Comment on lines
-87
to
+93
| self._filepath_or_buffer = cast(TextIOWrapper, self._filepath_or_buffer) # guaranteed by self._is_wrapped | ||
| self._filepath_or_buffer = cast( | ||
| TextIOWrapper, self._filepath_or_buffer | ||
| ) # guaranteed by self._is_wrapped |
JGSweets
commented
Oct 14, 2022
Comment on lines
-95
to
+103
| self._filepath_or_buffer = cast(IO, self._filepath_or_buffer) # can't be str due to conversion in __enter__ | ||
| self._filepath_or_buffer = cast( | ||
| IO, self._filepath_or_buffer | ||
| ) # can't be str due to conversion in __enter__ |
JGSweets
commented
Oct 14, 2022
| _data = _data.to_dict(orient="records", into=OrderedDict) | ||
| for i, sample in enumerate(_data): | ||
| _data[i] = json.dumps( | ||
| data = self._get_data_as_df(data) |
There was a problem hiding this comment.
no longer having two variables of the data with _data
fixes type acceptance at the top
JGSweets
commented
Oct 14, 2022
| """ | ||
| Extract the data as a json format. | ||
|
|
||
| :param data: raw data | ||
| :type data: list | ||
| :return: dataframe in json format | ||
| """ | ||
| _data: Union[pd.DataFrame, List] |
There was a problem hiding this comment.
no longer having two variables of the data with _data
fixes type acceptance at the top
JGSweets
commented
Oct 14, 2022
| @@ -349,6 +349,8 @@ def _convert_flat_to_nested_cols(cls, dic: Dict, separator: str = ".") -> Dict: | |||
| :return: | |||
| """ | |||
| for key in list(dic.keys()): | |||
| if not isinstance(key, str): | |||
There was a problem hiding this comment.
fix for allowing the [1] format in json reading
JGSweets
commented
Oct 14, 2022
| @@ -392,14 +394,16 @@ def is_match( | |||
| return True | |||
| except (json.JSONDecodeError, UnicodeDecodeError): | |||
| data_file.seek(0) | |||
|
|
|||
| json_identifier_re = re.compile(r"(:|\[)") | |||
There was a problem hiding this comment.
allows both : and [ as a JSON identifier differentiating from just a string.
JGSweets
commented
Oct 14, 2022
| @@ -12,11 +12,12 @@ | |||
|
|
|||
| class TestNestedJSON(unittest.TestCase): | |||
| def test_flat_to_nested_json(self): | |||
| dic = {"a.b": "ab", "a.c": "ac", "a.d.f": "adf", "b": "b"} | |||
| dic = {"a.b": "ab", "a.c": "ac", "a.d.f": "adf", "b": "b", 1: 3} | |||
There was a problem hiding this comment.
updates test to check for keys that aren't strings
JGSweets
commented
Oct 14, 2022
| @@ -56,6 +57,11 @@ def setUpClass(cls): | |||
| encoding="utf-8", | |||
| count=14, | |||
| ), | |||
| dict( | |||
There was a problem hiding this comment.
adds test which includes the new json format
JGSweets
commented
Oct 14, 2022
| @@ -0,0 +1,3 @@ | |||
| [1] | |||
There was a problem hiding this comment.
new data for testing json reading
ksneab7
approved these changes
Oct 17, 2022
taylorfturner
approved these changes
Oct 17, 2022
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.
Previously: we could not read line separated JSON arrays.
Now we can read:
becomes a JSONData which is wrapping pandas as: