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

Apply fixes for Segmentfactory and Pluginmount #54

Closed
mj0nez opened this issue Apr 16, 2023 · 4 comments
Closed

Apply fixes for Segmentfactory and Pluginmount #54

mj0nez opened this issue Apr 16, 2023 · 4 comments

Comments

@mj0nez
Copy link
Contributor

mj0nez commented Apr 16, 2023

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 23f218d 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
Copy link
Contributor

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
Copy link
Member

nerdoc commented Apr 17, 2023

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 😄

@mj0nez
Copy link
Contributor Author

mj0nez commented Apr 17, 2023

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
Copy link
Member

nerdoc commented Apr 22, 2023

#55 is merged. Closing this issue.

@nerdoc nerdoc closed this as completed Apr 22, 2023
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

No branches or pull requests

3 participants