Support `xs:list` inline lists, take 2 #38

ajoino · 2023-02-03T11:02:57Z

This is a redo of PR #37

I have addressed all comments expect the larger one about HeterogeneousSerializerFactory, I will create a separate PR for that

dapper91 · 2023-02-03T13:51:48Z

I think you branched off from master. Please rebase to dev because you PR brings commits from master branch.

ajoino · 2023-02-03T14:14:23Z

Rebased on dev so things should be in order now.

pydantic_xml/serializers.py

dapper91 · 2023-02-03T14:54:25Z

tests/test_homogeneous_collections.py

+    assert_xml_equal(actual_xml, xml)
+
+
+def test_text_tuple_extraction():


Seems like this test is redundant because is tests the same case as the previous one.

See comment below.

dapper91 · 2023-02-03T14:55:29Z

tests/test_homogeneous_collections.py

+    assert_xml_equal(actual_xml, xml)
+
+
+def test_attr_tuple_extraction():


the test is the same as previous.

I'm testing both tuple and list to make sure that they both work. But yes, they are essentially the same and if this test is covered by other tests I'm willing to remove it.

I mean they are literally identical. Based on the test name I think the model should be like this:

class RootModel(BaseXmlModel, tag="model"): values: Tuple[float] = attr()

dapper91 · 2023-02-03T14:57:35Z

tests/test_misc.py

@@ -39,7 +39,7 @@ class TestSubModel(BaseXmlModel, tag='model'):

    class TestModel(BaseXmlModel, tag='model'):
        model: TestSubModel
-        list: List[TestSubModel] = []
+        list: List[TestSubModel] = element(default=[])


What is the reason of that change?

Previously, that model would only have one possible text field, as List fields were implicitly elements. Now, without explicitly making a List type an element, it will be interpreted as the text. That would make that model have two Python fields that represent XML texts, model and list. Since the intention of the test was to have list represent an XML element, I changed to explicitly be so.

This is one of the backwards-incompatible changes I mentioned in the old PR.

Jacob-Flasheye · 2023-02-06T09:22:11Z

(This is @ajoino, accidentally commented using my work account again...)

Nice to see that 0.6.0 was released, though I'm sad I couldn't get this finished in time.
Going to rebase this branch with respect to the 0.6.0.

Regarding pre-commit:
I haven't used pre-commit in any projects before and I run into the following issue when running pre-commit install:

An unexpected error has occurred: CalledProcessError: command: ('/home/jacob/.cache/pre-commit/repolmlbeusl/py_env-python3/bin/python',
 '-mpip', 'install', '.')
return code: 1
stdout:
    Processing /home/jacob/.cache/pre-commit/repolmlbeusl
      Installing build dependencies: started
      Installing build dependencies: finished with status 'done'
      Getting requirements to build wheel: started
      Getting requirements to build wheel: finished with status 'done'
      Preparing metadata (pyproject.toml): started
      Preparing metadata (pyproject.toml): finished with status 'error'
    
stderr:
      error: subprocess-exited-with-error
      
      × Preparing metadata (pyproject.toml) did not run successfully.
      │ exit code: 1
      ╰─> [14 lines of output]
          Traceback (most recent call last):
            File "/home/jacob/.cache/pre-commit/repolmlbeusl/py_env-python3/lib/python3.10/site-packages/pip/_vendor/pep517/in_process/
_in_process.py", line 351, in <module>
              main()
            File "/home/jacob/.cache/pre-commit/repolmlbeusl/py_env-python3/lib/python3.10/site-packages/pip/_vendor/pep517/in_process/
_in_process.py", line 333, in main
              json_out['return_val'] = hook(**hook_input['kwargs'])
            File "/home/jacob/.cache/pre-commit/repolmlbeusl/py_env-python3/lib/python3.10/site-packages/pip/_vendor/pep517/in_process/
_in_process.py", line 152, in prepare_metadata_for_build_wheel
              return hook(metadata_directory, config_settings)
            File "/tmp/pip-build-env-b6bsl2th/overlay/lib/python3.10/site-packages/poetry/core/masonry/api.py", line 40, in prepare_met
adata_for_build_wheel
              poetry = Factory().create_poetry(Path(".").resolve(), with_groups=False)
            File "/tmp/pip-build-env-b6bsl2th/overlay/lib/python3.10/site-packages/poetry/core/factory.py", line 57, in create_poetry
              raise RuntimeError("The Poetry configuration is invalid:\n" + message)
          RuntimeError: The Poetry configuration is invalid:
            - [extras.pipfile_deprecated_finder.2] 'pip-shims<=0.3.4' does not match '^[a-zA-Z-_.0-9]+$'
          
          [end of output]
      
      note: This error originates from a subprocess, and is likely not a problem with pip.
    error: metadata-generation-failed
    
    × Encountered error while generating package metadata.
    ╰─> See above for output.
    
    note: This is an issue with the package mentioned above, not pip.
    hint: See above for details.

Something poetry-related given the logs but I don't use poetry for my own projects so it's hard to tell what's wrong.

dapper91 · 2023-02-06T09:29:09Z

(This is @ajoino, accidentally commented using my work account again...)
...
Something poetry-related given the logs but I don't use poetry for my own projects so it's hard to tell what's wrong.

Hi. It is a bug in isort pre-commit hook. I fixed that here a24d8a2, so you could rebase to that commit or run pre-commit autoupdate

Fixed review comments.

Added new tests and updated old tests as this addition is a regression/backwards incompatibility.

ajoino · 2023-02-14T20:25:37Z

I've updated to be up-to-speed with latest dev, but I rebased instead of merging and that goofed the history.

dapper91 · 2023-02-25T05:08:24Z

pydantic_xml/serializers/factories/homogeneous.py

+        ):
+            assert model_field.sub_fields and len(model_field.sub_fields) == 1
+            if (
+                isclass(model_field.type_) and issubclass(model_field.type_, pxml.BaseXmlModel) or


I extracted this logic to a function so that it can be reused.

Very nice!
Fixed this and the other comments.

dapper91 · 2023-02-25T05:27:15Z

pydantic_xml/serializers/factories/homogeneous.py

+
+            name = model_field.name
+
+            assert name is not None, "attr must be name"


according to pydantic name is not optional so I think this check is redundant.

dapper91 · 2023-02-25T05:29:37Z

pydantic_xml/serializers/factories/homogeneous.py

+
+            _, ns, nsmap = self._get_entity_info(model_field)
+
+            name = model_field.name


it is better to use model_field.alias to be compatible with pydantic aliased fields.

dapper91 · 2023-02-25T05:41:37Z

tests/test_homogeneous_collections.py

+    assert_xml_equal(actual_xml, xml)
+
+
+def test_attr_tuple_extraction():


I mean they are literally identical. Based on the test name I think the model should be like this:

class RootModel(BaseXmlModel, tag="model"): values: Tuple[float] = attr()

dapper91 · 2023-02-25T05:51:18Z

tests/test_homogeneous_collections.py

+    expected_obj = RootModel(
+        values=[1, 2, 70, -34],
+    )
+


assert actual_obj == expected_obj is missing I suppose

Ah, nice catch!
Yeah I must've gotten so happy changing the name of the test that I forgot to change the logic...

dapper91 · 2023-02-25T06:03:55Z

pydantic_xml/serializers/factories/homogeneous.py

+            text = element.pop_text()
+
+            if text is None:
+                return []


I think it is better to return None in this case otherwise default values will not work. For example for this model:

class RootModel(BaseXmlModel, tag="model"): values: Tuple[int, ...] = (0, 0)

if deserialize returns [] that code will not work because pydantic uses default values only if a value is not provided. But if deserialize returns [] it will not be filtered out by the model deserializer and the default value will not be set which is not expected behavior I suppose.

I don't know the best way to resolve multiple conversations on github but I've updated the PR with fixes to your comments.

I also realized recently that I probably should have created an issue first to discuss design and then made a PR. I apologize that I skipped that order and will do better next time. Thank you for your patience in dealing with me :)

codecov-commenter · 2023-03-06T09:39:50Z

Codecov Report

Merging #38 (09159f5) into dev (2754abd) will decrease coverage by 0.43%.
The diff coverage is 83.72%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev      #38      +/-   ##
==========================================
- Coverage   93.70%   93.28%   -0.43%     
==========================================
  Files          22       22              
  Lines        1033     1072      +39     
==========================================
+ Hits          968     1000      +32     
- Misses         65       72       +7

Flag	Coverage Δ
unittests	`93.28% <83.72%> (-0.43%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pydantic_xml/serializers/factories/homogeneous.py	`89.69% <83.72%> (-5.14%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ciaranfinn · 2023-03-07T13:41:06Z

tests/test_homogeneous_collections.py

+        values: List[float] = attr()
+
+    xml = '''
+    <model values="3.14 -1.0 300.0"/>


Will there be support for custom delimiters for attribute values? The space char seems to be the default for now, but curious if other delimiters will be supported in the future. I can imagine values might be separated by other chars like | and , .

# comma delimiter <model values="3.14,-1.0,300.0"/> # vertical bar delimiter. <model values="3.14|-1.0|300.0"/>

The current PR implements support for XML Schema lists, https://www.w3.org/TR/xmlschema11-2/#atomic-vs-list , which, afaict, only support white-space delimited values. I have no plans to implement other delimiters as this covers my (or rather my project $work's) needs. For other kinds of delimiters, I think you're better off using elements, as they are more general.

However, if someone need to consume lists with other delimiters, I'd gladly help with the implementation

Hey @ajoino, thank you for your reply. Yes the main issue I have is on the consumption side. At the moment my challenge is with the deserialization of the XML. I am hoping to be able to parse the pageReferences attribute and represent it as type List[int] instead of str. However, compared to your space delimited examples in this PR, I dealing with both | and , delimiters.

Example XML:

<?xml version="1.0" encoding="UTF-8"?> <books> <book id="1" name="BookA" pageReferences="2,6,14"/> <book id="2" name="BookB" pageReferences="1,8,57"/> </books>

Python Classes:

class Book(BaseXmlModel, tag="book"): id: str = attr(name="id") name: str = attr(name="name") page_references: str = attr(name="pageReferences") class BookResponse(BaseXmlModel, tag="books"): books: Optional[List[Book]] = element()

I was mainly wondering if it would be possible to pass a custom delimiter param to attr(). This could act as an override to the default space delimiter whenever it is specified. Alternatively you may have a better idea how to accomplish this, but this is just my reasoning for now.

class Book(BaseXmlModel, tag="book"): ... page_references: List[int] = attr(name="pageReferences", delimiter=",")

saharwenrix · 2024-09-16T14:09:02Z

is this going to be merged? support for xs:list is much needed

dapper91 · 2024-09-29T16:54:10Z

@saharwenrix Hi,

MRs to version 1 are no longer accepted, only critical bug fixes.
In version 2 xs:list can be handled using custom xml serializers: https://pydantic-xml.readthedocs.io/en/latest/pages/misc.html#custom-xml-serialization

ajoino mentioned this pull request Feb 3, 2023

Support xs:list inline lists #37

Closed

ajoino force-pushed the feature-inline-lists branch from 163969d to 6e333f2 Compare February 3, 2023 14:13

dapper91 reviewed Feb 3, 2023

View reviewed changes

ajoino added 5 commits February 14, 2023 21:09

Rebase branch to be up-to-date with dev.

6ba8825

Fixed review comments.

Fix merge conflict.

d6c038c

Add inline lists support for Attributes.

d075694

Added new tests and updated old tests as this addition is a regression/backwards incompatibility.

Add tests for homogeneous tuple.

6d8f38c

Fix formatting errors.

f21f0b8

ajoino force-pushed the feature-inline-lists branch from 6e333f2 to f21f0b8 Compare February 14, 2023 20:23

Fix name not being set/extracted by _entity_get_info.

4ca2f74

dapper91 reviewed Feb 25, 2023

View reviewed changes

Fix review comments.

09159f5

ciaranfinn reviewed Mar 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `xs:list` inline lists, take 2 #38

Support `xs:list` inline lists, take 2 #38

ajoino commented Feb 3, 2023

dapper91 commented Feb 3, 2023

ajoino commented Feb 3, 2023

dapper91 Feb 3, 2023

ajoino Feb 15, 2023

dapper91 Feb 3, 2023

ajoino Feb 14, 2023

dapper91 Feb 25, 2023

dapper91 Feb 3, 2023

ajoino Feb 14, 2023

Jacob-Flasheye commented Feb 6, 2023 •

edited

Loading

dapper91 commented Feb 6, 2023

ajoino commented Feb 14, 2023

dapper91 Feb 25, 2023

ajoino Mar 6, 2023

dapper91 Feb 25, 2023

dapper91 Feb 25, 2023

dapper91 Feb 25, 2023

dapper91 Feb 25, 2023

ajoino Mar 6, 2023

dapper91 Feb 25, 2023

ajoino Mar 6, 2023

codecov-commenter commented Mar 6, 2023

ciaranfinn Mar 7, 2023

ajoino Mar 9, 2023

ciaranfinn Mar 9, 2023 •

edited

Loading

saharwenrix commented Sep 16, 2024

dapper91 commented Sep 29, 2024

		assert_xml_equal(actual_xml, xml)


		def test_text_tuple_extraction():

		assert_xml_equal(actual_xml, xml)


		def test_attr_tuple_extraction():


		name = model_field.name

		assert name is not None, "attr must be name"


		_, ns, nsmap = self._get_entity_info(model_field)

		name = model_field.name

Support xs:list inline lists, take 2 #38

Are you sure you want to change the base?

Support xs:list inline lists, take 2 #38

Conversation

ajoino commented Feb 3, 2023

dapper91 commented Feb 3, 2023

ajoino commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacob-Flasheye commented Feb 6, 2023 • edited Loading

dapper91 commented Feb 6, 2023

ajoino commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciaranfinn Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

saharwenrix commented Sep 16, 2024

dapper91 commented Sep 29, 2024

Support `xs:list` inline lists, take 2 #38

Support `xs:list` inline lists, take 2 #38

Jacob-Flasheye commented Feb 6, 2023 •

edited

Loading

ciaranfinn Mar 9, 2023 •

edited

Loading