nerdocs / pydifact

A python EDIFACT library.
MIT License
156 stars 45 forks source link

Apply fixes for Segmentfactory and Pluginmount #54

Closed mj0nez closed 1 year ago

mj0nez commented 1 year ago

Hey, first, thanks for this amazing library, it has helped us a lot. I have noticed, that the in #37 proposed changes introduced more than a mapping functionality. With https://github.com/nerdocs/pydifact/pull/37/commits/23f218d246be5af3b86cd547b9710226ad58d019 fixes for the PluginMount and the SegmentFactory were introduced, which allow the inheritance based extension of Segment, while correctly evaluating the __omitted__ attribute.

With the current release this code

class TestSegment(Segment):

    tag = "TES"

    __omitted__ = False

    def validate(self) -> bool:
        pass

def test_has_plugin():
    plugins = SegmentProvider.plugins
    assert TestSegment in plugins

fails with

./tests/test_segment.py::test_has_plugin Failed: [undefined]assert TestSegment in []
def test_has_plugin():
        plugins = SegmentProvider.plugins
>       assert TestSegment in plugins
E       assert TestSegment in []

tests\test_segment.py:66: AssertionError

Could we merge those changes prior to the mapping extension? I will provide the necessary commits, but the credits belong to @theangryangel.

theangryangel commented 1 year ago

I know we’ve got people at work tinkering with the mapping stuff again. I’ll see if we can get it rebased on top of master if/when this gets merged in <3

nerdoc commented 1 year ago

So, if I understood correctly, best four you would be to merge this PR here, so @theangryangel & others could rebase work in #37 on top of that, right? For me this is ok - just give me your launch ok and I merge this PR. I'm busy myself in other projects, and am just a waitful watcher ATM here :smile:

mj0nez commented 1 year ago

Exactly! Publishing those changes would give me the opportunity to depend on them in another project, which was developed on top of #37. For now, I’m untangling some internal stuff but hopefully have a first release as well as another PR for pydifact soon. 😊

nerdoc commented 1 year ago

https://github.com/nerdocs/pydifact/pull/55 is merged. Closing this issue.