getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
218 stars 49 forks source link

Patch #499: Refactor find_host_config to lib.find_module_in_config #501

Closed BigRoy closed 4 years ago

BigRoy commented 4 years ago

This PR is related to #499

This now makes sure the way the config.{host} module is retrieved per host matches across each integration using avalon.lib.find_module_in_config as a replacement for the find_host_config function that each avalon.{host} module implemented.

Aside of that, I've made the variable names consistently config_host as opposed to replacing the config variable that could previously had left some confusion thinking it was the config module, but was actually the config.{host} mdoule.

And of course, as per #499 this should now allow config.{host} module to not exist at all for each of the integrations with only a logged warning along the lines of: Config has no '{config}.{host}' module.

Additionally this makes these changes:

Discussion

NOTE: This code should still be tested. However I wanted to set up this PR to discuss the code choices.

  1. Is the code clear enough?

  2. This imports the find_module_in_host function from avalon.lib as it became more readable than doing the following. However I believe this was frowned upon by the code style, but I couldn't find that mentioned anywhere.

    
    # in this PR
    from ..lib import find_module_in_host
    find_module_in_host(config, "maya")

the other option

import ..lib as avalon_lib avalon_lib.find_module_in_host(config, "maya")



3. Can we simplify further? Could the function be simplified to: `get_submodule`? This would shorten the function name and make it more generic. Then however, it logging the warning message feels to "specific" to the use cases. Maybe removing the warning is actually fine?
BigRoy commented 4 years ago

Actually, this could be done much easier.

We should remove the code from the individual host integrations and move it to the global install function. Will update the PR.

BigRoy commented 4 years ago

With https://github.com/getavalon/core/pull/501/commits/3f1aa64bc8e53cde982f09f67a213a13c2e03408 the Host Integrations now don't need to manually call install() and uninstall() on the configuration.

The {config}.{host}.install() and {config}.{host}.uninstall() are called from respectively avalon.pipeline.install() and avalon.pipeline.uninstall() now when the studio configuration has these methods exposed.

This avoids the need for the Integrations per host implementing this, simplifying the code greatly.

davidlatwe commented 4 years ago

However I believe this was frowned upon by the code style, but I couldn't find that mentioned anywhere.

I think it's here, in CONTRIBUTING # Code Quality - Architecture at row A8.

Could the function be simplified to: get_submodule?

Yeah, we can simplify that, but I lean to name it find_submodule, since I think the word get sounds like "You will get what you want for sure", and find is a bit uncertain which fit the scenario "You may not found the submodule you want, be careful".

Then however, it logging the warning message feels to "specific" to the use cases. Maybe removing the warning is actually fine?

I more like to keep the message, and rephrase it to like:

log_.warning("Could not find '%s' in module '%s'." % (submodule, module))

And the variables need to be renamed as well.

BigRoy commented 4 years ago

Thanks @davidlatwe for the thorough check. Admittedly I wrote the previous code at home and it was untested (as mentioned in the original post) because I was on holiday leave. Back in the office Today, cleaned up the code, listened to the lovely hound that is back to his regular woof-woof (thanks Marcus!) and tested the integrations in Maya, Houdini and Fusion to be sure. All good now.

Can you double check the code style and let me know if there's still anything out of the ordinary?

BigRoy commented 4 years ago

Just found this odd exception printing which is not a change by this PR but has been there for quite some time. Is there any Python version where that actually prints as a formatted string? I don't believe so.

Python 2

err = "bar"
print("Skipped: \"%s\" (%s)", mod_name, err)
# ('Skipped: "%s" (%s)', 'foo', 'bar')

Python 3

err = "bar"
print("Skipped: \"%s\" (%s)", mod_name, err)
# Skipped: "%s" (%s) foo bar

I'll patch that up too.

BigRoy commented 4 years ago

This is fixed with: https://github.com/getavalon/core/pull/501/commits/241e05d83e53ad128dbb3038dab493122cb3fea9

Retested in Maya + Houdini to be sure. All seems to work as expected. 👍

jasperges commented 4 years ago

@BigRoy Did a quick check with Blender with the changes you mentioned and everything seems to work fine.

BigRoy commented 4 years ago

Merging this.