Skip to content

Fix loader correctness and global side-effects from PR #103 review #121

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.

Sign up for GitHub

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

Merged
garlontas merged 1 commit into from
Apr 12, 2026
Merged

Fix loader correctness and global side-effects from PR #103 review #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions pystreamapi/loaders/__json/__json_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@ def generator():
# skipcq: PTC-W6004
with open(file_path, mode='r', encoding='utf-8') as jsonfile:
src = jsonfile.read()
if src == '':
if not src.strip():
return
yield from jsonlib.loads(src, object_hook=__dict_to_namedtuple)
result = jsonlib.loads(src, object_hook=__dict_to_namedtuple)
if isinstance(result, list):
yield from result
else:
yield result

return generator()

Expand All @@ -43,7 +47,11 @@ def __lazy_load_json_string(json_string: str) -> Iterator[Any]:
def generator():
if not json_string.strip():
return
yield from jsonlib.loads(json_string, object_hook=__dict_to_namedtuple)
result = jsonlib.loads(json_string, object_hook=__dict_to_namedtuple)
if isinstance(result, list):
yield from result
else:
yield result

return generator()

Expand Down
55 changes: 22 additions & 33 deletions pystreamapi/loaders/__xml/__xml_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,6 @@
from pystreamapi.loaders.__loader_utils import LoaderUtils


class __XmlLoaderUtil:
"""Utility class for the XML loader."""

def __init__(self):
self.cast_types = True
self.retrieve_children = True


config = __XmlLoaderUtil()


def xml(src: str, read_from_src=False, retrieve_children=True, cast_types=True,
encoding="utf-8") -> Iterator[Any]:
"""
Expand All @@ -38,70 +27,70 @@ def xml(src: str, read_from_src=False, retrieve_children=True, cast_types=True,
a path to an XML file.
:param cast_types: Set as False to disable casting of values to int, bool or float.
"""
config.cast_types = cast_types
config.retrieve_children = retrieve_children

if read_from_src:
return _lazy_parse_xml_string(src)
return _lazy_parse_xml_string(src, retrieve_children, cast_types)

path = LoaderUtils.validate_path(src)
return _lazy_parse_xml_file(path, encoding)
return _lazy_parse_xml_file(path, encoding, retrieve_children, cast_types)


def _lazy_parse_xml_file(file_path: str, encoding: str) -> Iterator[Any]:
def _lazy_parse_xml_file(file_path: str, encoding: str,
retrieve_children: bool, cast_types: bool) -> Iterator[Any]:
def generator():
with open(file_path, mode='r', encoding=encoding) as xmlfile:
xml_string = xmlfile.read()
yield from _parse_xml_string_lazy(xml_string)
yield from _parse_xml_string_lazy(xml_string, retrieve_children, cast_types)

return generator()


def _lazy_parse_xml_string(xml_string: str) -> Iterator[Any]:
def _lazy_parse_xml_string(xml_string: str, retrieve_children: bool,
cast_types: bool) -> Iterator[Any]:
def generator():
yield from _parse_xml_string_lazy(xml_string)
yield from _parse_xml_string_lazy(xml_string, retrieve_children, cast_types)

return generator()


def _parse_xml_string_lazy(xml_string: str) -> Iterator[Any]:
def _parse_xml_string_lazy(xml_string: str, retrieve_children: bool,
cast_types: bool) -> Iterator[Any]:
root = ElementTree.fromstring(xml_string)
parsed = __parse_xml(root)
if config.retrieve_children:
parsed = __parse_xml(root, cast_types)
if retrieve_children:
yield from __flatten(parsed)
else:
yield parsed


def __parse_xml(element):
def __parse_xml(element, cast_types: bool):
"""Parse XML element and convert it into a namedtuple."""
if len(element) == 0:
return __parse_empty_element(element)
return __parse_empty_element(element, cast_types)
if len(element) == 1:
return __parse_single_element(element)
return __parse_multiple_elements(element)
return __parse_single_element(element, cast_types)
return __parse_multiple_elements(element, cast_types)


def __parse_empty_element(element):
def __parse_empty_element(element, cast_types: bool):
"""Parse XML element without children and convert it into a namedtuple."""
return LoaderUtils.try_cast(element.text) if config.cast_types else element.text
return LoaderUtils.try_cast(element.text) if cast_types else element.text


def __parse_single_element(element):
def __parse_single_element(element, cast_types: bool):
"""Parse XML element with a single child and convert it into a namedtuple."""
sub_element = element[0]
sub_item = __parse_xml(sub_element)
sub_item = __parse_xml(sub_element, cast_types)
Item = namedtuple(element.tag, [sub_element.tag])
return Item(sub_item)


def __parse_multiple_elements(element):
def __parse_multiple_elements(element, cast_types: bool):
"""Parse XML element with multiple children and convert it into a namedtuple."""
tag_dict = {}
for e in element:
if e.tag not in tag_dict:
tag_dict[e.tag] = []
tag_dict[e.tag].append(__parse_xml(e))
tag_dict[e.tag].append(__parse_xml(e, cast_types))
filtered_dict = __filter_single_items(tag_dict)
Item = namedtuple(element.tag, filtered_dict.keys())
return Item(*filtered_dict.values())
Expand Down
17 changes: 10 additions & 7 deletions tests/_loaders/test_xml_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@

class TestXmlLoader(TestCase):

def setUp(self):
self.file_content = file_content

@contextmanager
def mock_csv_file(self, content=None, exists=True, is_file=True):
"""Context manager for mocking CSV file operations.
def mock_xml_file(self, content=None, exists=True, is_file=True):
"""Context manager for mocking XML file operations.

Args:
content: The content of the mocked file
Expand All @@ -48,7 +51,7 @@ def mock_csv_file(self, content=None, exists=True, is_file=True):
yield

def test_xml_loader_from_file_children(self):
with self.mock_csv_file(file_content):
with self.mock_xml_file(file_content):
data = xml(file_path)

first = next(data)
Expand All @@ -66,7 +69,7 @@ def test_xml_loader_from_file_children(self):
self.assertRaises(StopIteration, next, data)

def test_xml_loader_from_file_no_children_false(self):
with self.mock_csv_file(file_content):
with self.mock_xml_file(file_content):
data = xml(file_path, retrieve_children=False)

first = next(data)
Expand All @@ -80,7 +83,7 @@ def test_xml_loader_from_file_no_children_false(self):
self.assertRaises(StopIteration, next, data)

def test_xml_loader_no_casting(self):
with self.mock_csv_file(file_content):
with self.mock_xml_file(file_content):
data = xml(file_path, cast_types=False)

first = next(data)
Expand All @@ -98,12 +101,12 @@ def test_xml_loader_no_casting(self):
self.assertRaises(StopIteration, next, data)

def test_xml_loader_is_iterable(self):
with self.mock_csv_file(file_content):
with self.mock_xml_file(file_content):
data = xml(file_path)
self.assertEqual(len(list(iter(data))), 3)
Comment on lines 103 to 106

@sourcery-ai sourcery-ai Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding tests for XML loader when using read_from_src=True with different flag combinations

With the refactor removing the global config and threading flags through the call chain, the read_from_src=True path (_lazy_parse_xml_string) should be covered similarly to the file-path loader.

Could you add tests that:

  • Call xml(xml_string, read_from_src=True) with retrieve_children set to both True and False, checking the parsed data shape matches the existing file-based tests.
  • Call xml(xml_string, read_from_src=True, cast_types=False) and assert numeric/boolean-like values remain strings, mirroring test_xml_loader_no_casting.

You can reuse the existing file_content XML string so both entry points (file_path and read_from_src) stay aligned without much extra test code.

Suggested implementation:

    def test_xml_loader_no_casting(self):
        with self.mock_xml_file(file_content):
            data = xml(file_path, cast_types=False)

            first = next(data)
            self.assertRaises(StopIteration, next, data)

    def test_xml_loader_from_src_retrieve_children_true(self):
        # read_from_src=True should behave the same as the file-path loader
        with self.mock_xml_file(file_content):
            file_data = list(xml(file_path, retrieve_children=True))

        src_data = list(xml(file_content, read_from_src=True, retrieve_children=True))
        self.assertEqual(src_data, file_data)

    def test_xml_loader_from_src_retrieve_children_false(self):
        # read_from_src=True with retrieve_children=False should mirror file-path behaviour
        with self.mock_xml_file(file_content):
            file_data = list(xml(file_path, retrieve_children=False))

        src_data = list(xml(file_content, read_from_src=True, retrieve_children=False))
        self.assertEqual(src_data, file_data)

    def test_xml_loader_from_src_no_casting(self):
        # read_from_src=True with cast_types=False should mirror test_xml_loader_no_casting
        with self.mock_xml_file(file_content):
            file_data = list(xml(file_path, cast_types=False))

        src_data = list(xml(file_content, read_from_src=True, cast_types=False))
        self.assertEqual(src_data, file_data)

    def test_xml_loader_is_iterable(self):
        with self.mock_xml_file(file_content):
            data = xml(file_path)
            self.assertEqual(len(list(iter(data))), 3)

    def test_xml_loader_with_empty_file(self):
        with self.mock_xml_file(''):
            data = xml(file_path)
            self.assertRaises(ParseError, next, data)

These changes assume:

  1. The xml loader already accepts read_from_src and retrieve_children keyword arguments, matching the refactor you mentioned.
  2. file_content is the XML string used in other tests in this module, and file_path/mock_xml_file are available helpers as shown.

If there are existing dedicated tests for retrieve_children=True/False with the file-path loader earlier in this file, these new tests now assert that the read_from_src=True path produces identical output to the file-based path for the same flags, without needing to duplicate structure-specific assertions.


def test_xml_loader_with_empty_file(self):
with self.mock_csv_file(''):
with self.mock_xml_file(''):
data = xml(file_path)
self.assertRaises(ParseError, next, data)

Expand Down
7 changes: 7 additions & 0 deletions tests/_loaders/test_yaml_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from unittest import TestCase
from unittest.mock import patch, mock_open

import yaml as yaml_lib

from _loaders.file_test import OPEN, PATH_EXISTS, PATH_ISFILE
from pystreamapi.loaders import yaml

Expand Down Expand Up @@ -62,6 +64,11 @@ def test_yaml_loader_is_lazy(self):
data = yaml(file_path)
self.assertIsInstance(data, GeneratorType)

def test_yaml_loader_with_malformed_yaml(self):
malformed_yaml = "key: : invalid"
with self.assertRaises(yaml_lib.YAMLError):
list(yaml(malformed_yaml, read_from_src=True))

def _check_extracted_data(self, data):
Comment on lines +67 to 72

@sourcery-ai sourcery-ai Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Extend malformed YAML coverage to the file-based loader path

You’ve covered the read_from_src=True path. To keep behavior consistent with the other loader tests, please add a file-based variant that:

  • Writes the same malformed YAML to a temp file (or uses the existing file-mocking helper).
  • Calls yaml(file_path) without read_from_src=True.
  • Asserts yaml_lib.YAMLError is raised when the generator is consumed.

This will verify that both string and file inputs fail the same way for invalid YAML.

Suggested change
def test_yaml_loader_with_malformed_yaml(self):
malformed_yaml = "key: : invalid"
with self.assertRaises(yaml_lib.YAMLError):
list(yaml(malformed_yaml, read_from_src=True))
def _check_extracted_data(self, data):
def test_yaml_loader_with_malformed_yaml(self):
malformed_yaml = "key: : invalid"
with self.assertRaises(yaml_lib.YAMLError):
list(yaml(malformed_yaml, read_from_src=True))
def test_yaml_loader_with_malformed_yaml_file_path(self):
malformed_yaml = "key: : invalid"
with patch(PATH_EXISTS, return_value=True), \
patch(PATH_ISFILE, return_value=True), \
patch(OPEN, mock_open(read_data=malformed_yaml)):
with self.assertRaises(yaml_lib.YAMLError):
list(yaml("malformed.yaml"))
def _check_extracted_data(self, data):

first = next(data)
self.assertEqual(first.attr1, 1)
Expand Down
Loading