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

Duplicate plugin names should be supported. #332

Merged
merged 17 commits into from Jan 24, 2018

Conversation

Projects
None yet
3 participants
@tokejepsen
Member

tokejepsen commented Jan 23, 2018

This is just the test to show that duplicate plugin names are not supported, so the tests should fail.

I'm pretty sure the issue is here, because we compare the plugin names rather than their class string:

<class 'c:\users\tje\appdata\local\temp\tmponhxwt\pluginA.py.collectorA'>

What I could not find was where we set this string?

@tokejepsen tokejepsen requested a review from mottosso Jan 23, 2018

@mottosso

This comment has been minimized.

Member

mottosso commented Jan 23, 2018

Hm, should this really be supported? The thing we implemented support for last time around was duplicate file names.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

Hm, should this really be supported? The thing we implemented support for last time around was duplicate file names.

The use-case is when you have lots of plugins from different sources, it can be tricky to get a unique name that is descriptive and short.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

The logic for supporting duplicate names are there, but api.discover() filters duplicate plugin names.

@mottosso

This comment has been minimized.

Member

mottosso commented Jan 23, 2018

Ok, welcome to remove that check. If the tests hold, then I can't see why not.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

There is a single failing test, which is a direct contradiction to what we are trying to achieve here:

@with_setup(setup_duplicate, teardown)
def test_no_duplicate_plugins():
"""Discovering plugins results in a single occurence of each plugin"""
plugin_paths = pyblish.plugin.plugin_paths()
assert_equals(len(plugin_paths), 2)
plugins = pyblish.plugin.discover(type='selectors')
# There are two plugins available, but one of them is
# hidden under the duplicate module name. As a result,
# only one of them is returned. A log message is printed
# to alert the user.
assert_equals(len(plugins), 1)

This a test for pre11, so the question is how to handle this?
We could remove the test, but that'll break backwards compatibility. We could also have a check for duplicate plugin names for pre11 versions only.

@mottosso

This comment has been minimized.

Member

mottosso commented Jan 23, 2018

Ok, in that case we'll need to make it opt-in.

Such as an environment var.

$ set PYBLISH_ALLOW_DUPLICATE_PLUGINS=True

Then for 2.0 we could make it default. Not ideal, nor clean. But backwards compatibility is king.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

I opted for PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES in the end.

The code might be a little verbose, so let me know what you think.

continue
# Check for duplicate plugin names. This is to preserve
# backwards compatility.
allow_duplicates = eval(

This comment has been minimized.

@mottosso

mottosso Jan 23, 2018

Member

What's eval for?

I'd suggest..

ALLOW_DUPLICATES = bool(os.getenv("PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES"))

At the header of the module and..

                    if not ALLOW_DUPLICATES and plugin.__name__ in plugin_names:
                        log.debug("Duplicate plug-in found: %s", plugin)
                        continue

As a change to the body.

This comment has been minimized.

@tokejepsen

tokejepsen Jan 23, 2018

Member

What's eval for?

allow_duplicates = bool(os.getenv("PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES")) does not evaluate the string to a boolean. For example bool("False") is True.

This comment has been minimized.

@mottosso

mottosso Jan 23, 2018

Member

Aah, ok, no that's fine. If the variable exists, no matter its value, then it's True. Common practice.

tokejepsen added some commits Jan 23, 2018

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

Ready to merge?

@mottosso

This comment has been minimized.

Member

mottosso commented Jan 23, 2018

No, need to get rid of the eval stuff.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jan 23, 2018

No, need to get rid of the eval stuff.

Thought I did?

if plugin.__name__ in plugins:
# Check for duplicate plugin names. This is to preserve
# backwards compatility.
allow_duplicates = bool(

This comment has been minimized.

@mottosso

mottosso Jan 23, 2018

Member

This can be evaluated on module-import, no need to do it every iteration of this loop.

This comment has been minimized.

@tokejepsen

tokejepsen Jan 23, 2018

Member

The issue with putting it at the module level, is that the tests can't change the behaviour per test.

This comment has been minimized.

@tokejepsen

tokejepsen Jan 23, 2018

Member

How about the latest?
Its once at the function level.

This comment has been minimized.

@mottosso

mottosso Jan 23, 2018

Member

Ah, I see what you mean. Well, you can always do module.THIS_VARIABLE = True for testing's sake. I think that should be fine.

This comment has been minimized.

@tokejepsen

tokejepsen Jan 24, 2018

Member

Good point. Done.

tokejepsen and others added some commits Jan 23, 2018

@mottosso

This comment has been minimized.

Member

mottosso commented Jan 24, 2018

Ok, I'm happy. Merge when ready. 👍

@tokejepsen tokejepsen merged commit 5f26610 into pyblish:master Jan 24, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 95.238%
Details

@tokejepsen tokejepsen deleted the tokejepsen:duplicate_plugin_names branch Jan 24, 2018

@tokejepsen tokejepsen referenced this pull request Mar 19, 2018

Open

Implement #250 #325

@RafaelVillar

This comment has been minimized.

RafaelVillar commented Jun 8, 2018

Hello, in regards of the duplicate plugins of the same name.
Doesn't the below always default to True no matter the string?

ALLOW_DUPLICATES = bool( os.getenv("PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES") )

True = bool("blah"), bool("false"), bool("False")

vs

ALLOW_DUPLICATES = ast.literal_eval( os.getenv("PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES") )
False = bool("False")
True = bool("True")

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jun 8, 2018

Hey @RafaelVillar

You are right. Once the environment variable PYBLISH_ALLOW_DUPLICATE_PLUGIN_NAMES is available, no matter what that variable is, it'll evaluate as True.

@mottosso

This comment has been minimized.

Member

mottosso commented Jun 8, 2018

That's right, any value == True. It's by design and aligns with how boolean environment variables are implemented elsewhere, like Maya.

@RafaelVillar

This comment has been minimized.

RafaelVillar commented Jun 11, 2018

Thank you for pointing that out. I suppose I didnt think of it like that. Not my initial preference, but good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment