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

Fix/issue 26 : YAML Parsing for Large Config Files #28

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

LucasDedieu
Copy link
Contributor

@LucasDedieu LucasDedieu commented Dec 16, 2024

Fix YAML Parsing for Large Config Files

Description

Support for YAML config files exceeding 4096 characters:
Previously, the library failed to handle YAML files larger than 4096 characters due to limitations in how values were parsed in ConfitYamlLoader. This has been resolved by modifying the construct_object method to use x.value, ensuring robust parsing regardless of file size.

Key Changes:

  • Updated ConfitYamlLoader to use x.value for parsing scalar nodes.
  • Added test_very_long_yaml_config and test_escaped_string unit tests.

Checklist

  • If this PR is a bug fix, the bug is documented in the test suite.
  • Changes were documented in the changelog (pending section).
  • If necessary, changes were made to the documentation.

Copy link

github-actions bot commented Dec 16, 2024

Coverage Report

NameStmtsMiss∆ MissCover
TOTAL95718098.12%
Files without new missing coverage
NameStmtsMiss∆ MissCover
confit/utils/settings.py

Was already missing at lines 10-12

         return bool(loads(value))
-     except Exception:
-         return True

102080.00%
confit/registry.py

Was already missing at line 119

         if name is None and hasattr(e.model, "type_"):
-             name = e.model.type_.__module__ + "." + e.model.type_.__qualname__
         e = ConfitValidationError(
Was already missing at lines 479-484
         """
-         entrypoints = importlib_metadata.entry_points()
  ...
-             return entrypoints.get(self.entry_point_namespace, [])
Was already missing at line 509
                 if from_entry_point:
-                     return from_entry_point
             if not catalogue.check_exists(*path):
Was already missing at line 516
                 )
-             return catalogue._get(path)

2147096.73%
confit/config.py

Was already missing at line 126

                     else:
-                         current = current[part]
                 try:
Was already missing at line 155
         if resolve:
-             return config.resolve(registry=registry)
Was already missing at line 377
                     return local_names[parts] + ("." if path.endswith(":") else "")
-                 except KeyError:
                     raise KeyError(path)
Was already missing at line 422
             if not deep and len(loc) > 1:
-                 return obj
Was already missing at line 502
                 return
-             current[path] = val
Was already missing at line 529
                     ):
-                         old[key] = new_val
                     else:
Was already missing at line 565
             elif isinstance(obj, list):
-                 return [rec(v) for v in obj]
             elif isinstance(obj, tuple):
Was already missing at line 567
             elif isinstance(obj, tuple):
-                 return tuple(rec(v) for v in obj)
             elif isinstance(obj, Reference):
Was already missing at line 616
     if isinstance(config_paths, Path):
-         config_paths = [config_paths]

3069097.06%

8 files skipped due to complete coverage.

Coverage success: total of 98.12% is above 98.12% 🎉

@percevalw
Copy link
Member

Thank you @LucasDedieu ! I think there is a unwanted side effect to your PR, which was not tested already:

def test_escaped_string():
    config = Config.from_yaml_str("""
section:
    num: 1
    escaped_num: "1"
    escaped_broken_ref: "${test.a"
    escaped_ref: "${test.a}"
""").resolve(registry=registry)
    assert config["section"]["num"] == 1
    assert config["section"]["escaped_num"] == "1"
    assert config["section"]["escaped_broken_ref"] == "${test.a"
    assert config["section"]["escaped_ref"] == "${test.a}"

We want users to be able to "escape" references (or anything else), if what is they really want is to write a string containing ${...}. Could you add this test and verify that your changes pass it ?

Copy link
Member

@percevalw percevalw left a comment

Choose a reason for hiding this comment

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

Perfect !

@LucasDedieu LucasDedieu merged commit f5bae0d into main Dec 17, 2024
10 checks passed
@LucasDedieu LucasDedieu deleted the fix/issue_26 branch December 17, 2024 13:34
@LucasDedieu LucasDedieu linked an issue Dec 17, 2024 that may be closed by this pull request
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.

YAML config files over 4096 chars
2 participants