fix nbconvert filter validation

[jakevdp]

Fixes issue #3505

[jakevdp]

It still might be cleaner to replace this block with a call to register_filter, but there are some differences between the two that may be deliberate. For example, is there a reason the code here is checking for a subclass of MetaHasTraits rather than an instance of MetaHasTraits?

[minrk]

I think it's checking the subclass because config will specify that as a class, rather than an instance of that class. This isn't how most of IPython works when specifying classes for things, so there might be a better way.

[Carreau]

It still might be cleaner to replace this block with a call to register_filter, but there are some differences between the two that may be deliberate. For example, is there a reason the code here is checking for a subclass of MetaHasTraits rather than an instance of MetaHasTraits?

Hi jake, Sorry I was not able to respond to you earlier, and is not able to improve nbconvert a lot those day.
I'm not sure there is a reason to check for isSubClass rather than isInstance ( I still need to look again on how those will work with Configurable). At least there are no reason to treat predefined and user filter differently.

[jakevdp]

As I think about it more, I think the issubclass call is in error. Why would you ever pass a subclass of a metaclass as a filter? An instance of a metaclass is a class, so we should be checking for isinstance here.

[minrk]

ok, then go ahead and make the change. I was thinking of it as the plain HasTraits, when really the logic guarded by the check here expects issubclass(Configurable).

[Carreau]

Agreed with Min. issubclass is a mistake, if you have time, adding tests is a welcomed addition :-)

[minrk]

I think @jdfreder is getting started putting the test suite together, so maybe wait for that before committing a test to avoid conflict.

[Carreau]

I think @jdfreder is getting started putting the test suite together, so maybe wait for that before committing a test to avoid conflict.

Ah, my bad, I though the test were added before merging. @jakevdp apparently pushed new commits a few minutes ago that seem ok to me.

[minrk]

Yup, fine for now.

[Carreau]

Ok, merging.