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

Validate publish data type #324

Merged
merged 13 commits into from Aug 1, 2018

Conversation

Projects
None yet
2 participants
@tokejepsen
Member

tokejepsen commented Sep 4, 2017

Motivation

When assigning any types other than boolean to an instance's data member publish it'll always return True.

The data member have to be a boolean to work correctly.

Implementation

The solution is to add a built-in validator to validate all instances' publish data member.

@tokejepsen tokejepsen requested a review from mottosso Sep 4, 2017

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 24, 2018

Looks like Appveyor is failing on the support for Python 2.6 is ended for nose?

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 25, 2018

Yeap, looks like it. If you delete the 2.6 entries from here it should pass. No need to test against it anymore.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 25, 2018

Good to merge?

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 25, 2018

Please don't merge yet, I'm not sure about this functionality.

When assigning any types other than boolean to an instance's data member publish it'll always return True.

What's wrong with that? Have you been seeing errors because of it (e.g. in Pyblish QML) or is it something logically confusing or..?

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 25, 2018

The issue was that an application (think it was Nuke) was returning a string rather than a boolean.
Without knowing this logic within the application I spend ages trying to figure out why when mapping an attribute to publish was always evaluating to True.
This is purely to help someone else in the future not spend the same amount of time on this issue.

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 29, 2018

Ok, a little unsure of this one still; my problem with it is that it covers an edge case at the cost of one more permanent validation step. I'm a little weary of obscuring the end-users validators with ones that are for developers, rather than the artists.

I think the idea (of integrity checking ones own data) is solid and that there is a place for it.. I just wonder whether this is it. It is somewhat occupying the end-user space when it should really be something only the developer sees and can do something about.

Should we add override __setitem__ on instance.data and perform sanity checking "live" when the setting of the attribute actually happens?

instance.data["publish"] = "Wrong"
# ERROR!

I dunno, what do you think?

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 31, 2018

That could work :)

I've implemented it so the data member on both context and instance have this validation. Could split it up, but that would introduce maybe too much unnecessary code.

# Validate "publish" data member to always be boolean
if k == "publish":
msg = "\"publish\" data member has to be boolean."
assert isinstance(v, bool), msg

This comment has been minimized.

@mottosso

mottosso Jul 31, 2018

Member

Would a TypeError be more intuitive here?

This comment has been minimized.

@tokejepsen

tokejepsen Jul 31, 2018

Member

Good point! Done.

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 31, 2018

Sweet. I think this is quite slick. Let's give this a try, and potentially we could add validation for more types, and potentially add a plug-in-type system for end-users to validate their own types too.

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 31, 2018

There is one thing however.. this is a backwards breaking change, so I think we must put it behind an environment variable pre-2.0. Do you remember what the last breaking change was, that we put behind a variable? Probably safe to use them same. Then we can start gathering changes to make for a 2.0.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 31, 2018

this is a backwards breaking change

I was wondering whether this would qualify as breaking?
This means you have to have publish data member as boolean. But I would argue that users already have it as boolean, since that is what pyblish-qml and pyblish-lite needs.

Do you remember what the last breaking change was, that we put behind a variable?

That would be PYBLISH_ALLOW_DUPLICATE_PLUGINS in https://github.com/pyblish/pyblish-base/releases/tag/1.6.1

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 31, 2018

I was wondering whether this would qualify as breaking?

I can see what you mean, but if you put yourself in the shoes of someone updating Pyblish - not specifically for this feature, but just a general "I want the latest" - then he may discover that now things no longer work. He'll get a TypeError, even though he hasn't changed anything.

That's what makes it a backwards incompatible change.

Now, arguably, he shouldn't have used a non-Bool for this value so shouldn't be seeing this error. But that's what happens when you used to allow something and now don't. Bam!

PYBLISH_ALLOW_DUPLICATE_PLUGINS

That's excellent, I think it's time we step this up.

  1. Let's put this change under a PYBLISH_STRICT_DATATYPES
  2. And override both (i.e. enable both) with a PYBLISH_EARLY_ADOPTER

Such that enabling either variable means enabling either feature independently (for fine control) and enabling early-adopter gets you what 2.0 will eventually look like.

That way, we can keep making incompatible changes and also remove deprecated stuff, so long as it only breaks things explicitly, by setting variables such as these.

Example

# Fine control
ALLOW_DUPLICATE_PLUGINS = bool(os.getenv("PYBLISH_ALLOW_DUPLICATE_PLUGINS"))
STRICT_DATATYPES = bool(os.getenv("PYBLISH_STRICT_DATATYPES"))

# Global override for 2.0 stuff
EARLY_ADOPTER = bool(os.getenv("PYBLISH_EARLY_ADOPTER"))
ALLOW_DUPLICATE_PLUGINS = EARLY_ADOPTER or ALLOW_DUPLICATE_PLUGINS
STRICT_DATATYPES = EARLY_ADOPTER or STRICT_DATATYPES 

I know this falls a little outside of this issue, we could either merge this as-is, not release it, and make another issue for these features. Or do them now. Up to you.

@tokejepsen

This comment has been minimized.

Member

tokejepsen commented Jul 31, 2018

Think that sounds good :) Done.

@@ -666,6 +671,15 @@ def __call__(self, key=None, default=None):
return self.get(key, default)
def __setitem__(self, k, v):
# Backwards incompatible data validation.
if STRICT_DATATYPES:

This comment has been minimized.

@mottosso

mottosso Jul 31, 2018

Member

For perfect compatibility, you could put the if-statement outside of the method declaration. That way, the override would only happen for early adopters, avoiding potential edge-cases where overriding this causes trouble.

def test_validate_publish_data_member_type():
"""Validate publish data member type works."""
pyblish.plugin.STRICT_DATATYPES = True

This comment has been minimized.

@mottosso

mottosso Jul 31, 2018

Member

..Although, that would limit your ability to do this.

Let's leave it. Looks good!

@mottosso

This comment has been minimized.

Member

mottosso commented Jul 31, 2018

Genius! Looking good, merge at will!

@tokejepsen tokejepsen merged commit 3724b0f into pyblish:master Aug 1, 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 Coverage increased (+0.04%) to 95.213%
Details

@tokejepsen tokejepsen deleted the tokejepsen:validate_publish_data_type branch Aug 1, 2018

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