Add 'context' key to 'result' dict

pyblish / pyblish-base

Pyblish base library - see https://github.com/pyblish/pyblish for details.

Other

127 stars 59 forks source link

Add 'context' key to 'result' dict #401

Closed MustafaJafar closed 5 months ago

MustafaJafar commented 6 months ago

Changelog Description

This PR adds context key to result dict inside plugin.process

Additional Info 1

This PR is very helpful for people who develop some debugging tools like https://gist.github.com/BigRoy/1972822065e38f8fae7521078e44eca2

This PR avoids the need to create custom redundant events.

Additional Info 2

Things I tried:

Accessing context via result["instance"].context but this only works for instance plugins.
Passing the context as argument to lib.emit -> lib.emit("pluginProcessed", result=result, context=context) but apparently this affects every single call so we will get an error TypeError: on_plugin_processed() got an unexpected keyword argument 'context'
The backward friendly solution was just to update the result key.

BigRoy commented 6 months ago

Yes - this would be great.

This would make context easily accessible from the pluginProcessed event callback which is the reason you're making this change but wasn't extremely apparent from the PR description.

mottosso commented 5 months ago

Sorry for the delay I must have forgot to hit "Comment".

This should work fine, my only concern is with cyclic dependencies. I recall there was a reason for not including this, but can't recall exactly what. It did involve recursion and memory bloating. If you can confirm that no references to itself exist in the result that would help alleviate this issue. For example, the instance contains reference to the context, which contains reference to each instance.. I can't be sure. Can we list out the dependencies here so we know?

mottosso commented 5 months ago

result["instance"].context

Mm, true, this is already a cycle, as the context then also includes the instance which includes the context etc. So it mustn't be an issue to add another then.

Then this looks fine to me, I'm happy to merge this once you are happy.

MustafaJafar commented 5 months ago

Then this looks fine to me, I'm happy to merge this once you are happy.

Great, thanks a ton! I'm really happy indeed.

~~Would you recommend the previous suggestions ? Or the PR as it's looks good ?~~ Oh, My question was already answered!

So it mustn't be an issue to add another then.