Fix bad hooks

[heavyimage]

After a long wait, we're trying to leap our custom hiero exporter forward a bit to bring in the latest updates.

It was with some surprise that upon checking out v0.5.1, it flat out had bugs in it. Only by applying these commits we get the exporter working again.

[houndci-bot]

expected 2 blank lines, found 1

[houndci-bot]

expected 2 blank lines, found 1

[thebeeland]

Hey Jesse, we're curious why you needed to make these changes? There are a few things you've done here, and I don't quite understand why:

1. You've switched to subclassing the Hook base class directly, rather than using the get_hook_baseclass method provided by tk-core. Is this because you're using a tk-core that's older than that method?
2. You moved all of the logic from the base_hooks into the app's public hook interface. This will break our documentation generation code, and works against the more recent approach to hook structure we've been using in recent years. I'm assuming this was required as a result of the first item on this list, as the base_hooks inheritance will have been broken by directly subclassing the Hook base class.
3. You've added some **kwargs params to the hook methods. Do you have custom app code (beyond just modified hooks) that's causing additional arguments to be passed to the hooks that we don't support in our release of the app? If so, what would those be, and would it make more sense for us to support those directly rather than allowing arbitrary kwargs to be passed to the hook methods? We would prefer a more explicit, strictly-enforced interface if possible.

[heavyimage]

Hey Jeff,

Sorry -- I tried to explain my reasoning in each commit's commit message and not in the overall PR; that was perhaps dumb. Does reading those commit messages help at all?

To reply to your points:

1. This was just cosmetic; If I checkout v0.5.1, 4 of the 12 hooks seem to find the hook baseclass via the get_hook_baseclass() method and 8 just use from tank import Hook. I thought it was odd that there was a difference since I assumed the functionality was the same? Is that incorrect? In v0.4.3 only 1 of 9 uses the get_hook_baseclass() method. This is the change made by my 090e02 commit.

2. I swapped the hiero_customize_export_ui.py hook in hooks with the one in python/base_hooks (in the 1bfd3 commit) because I thought perhaps they had been swapped with each other in your release of v0.5.1. My reasoning for that is that during the init of ShotgunTranscodePreset on line ~468 of python/tk_hiero_export/version_creator.py:

    # Handle custom properties from the customize_export_ui hook.
    custom_properties = self._get_custom_properties(
        "get_transcode_exporter_ui_properties"
    ) or []
   

which calls line ~64 of python/tk_hiero_export/base.py:

     self._custom_property_definitions[get_method] = self.app.execute_hook_method(
            "hook_customize_export_ui",
            get_method,
            base_class=HieroCustomizeExportUI,
        )

This reduces down to a call to a function called get_transcode_exporter_ui_properties() in the customize_export_ui hook which doesn't exist and errors -- the provided hook only defines a class but no methods inside it like get_transcode_exporter_ui_properties(). The only implementation (really just a signature) I could find for this function was in the hook inside of python/base_hooks. Without this change, the exporter fails I believe on startup since ShotgunTranscodePreset fails to init. Unlike point 1 above, this seems to be a non-cosmetic, DOA problem, no?

3. Because all the calls to the ui customization hook pass along a "base_class" argument (as seen above in the snippit from base.py, for instance), any of the many calls to the functions in that hook (which again, per point 2 above are missing) fail because the functions receive an extra argument of base_class; I originally added a base_class keyword arg to each of the methods required in the customize_export_ui hook but then decided on the more generic **kwargs especially since many of the other hooks use that instead I guess for future proofing? This is from the 9a5f17 commit.

TL;DR I may have made things more difficult for myself by using the python/base_hooks/hiero_customize_export_ui.py file as a starting point but I guess I only got there because the HieroCustomizeExportUI class defined in the hooks/hiero_customize_export_ui.py doesn't have stubbed out functions for each of the ones referred to in the app which means it errors on startup.

[heavyimage]

Hey -- any updates on this?