Skip to content
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

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

Merged
merged 11 commits into from
Jan 7, 2020

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Dec 31, 2019

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:

  • Add uninstall() function to avalon.fusion as that was missing.
  • Match Houdini and Fusion metadata collecting to Maya implementation, which had an optimization.

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")
  1. 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?

- Add `uninstall()` function to `avalon.fusion` as that was missing.
- Match Houdini and Fusion metadata collecting to Maya implementation, which had an optimization.
@BigRoy
Copy link
Collaborator Author

BigRoy commented Dec 31, 2019

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
Copy link
Collaborator Author

BigRoy commented Dec 31, 2019

With 3f1aa64 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.

@BigRoy BigRoy requested a review from mkolar December 31, 2019 10:21
Copy link
Collaborator

@davidlatwe davidlatwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work ! Code simplified !
Just a few place need to be polished ;)
And looks like the import statements in each hosts' pipeline.py can be polished as well, since some of the modules are not used anymore.

avalon/lib.py Outdated Show resolved Hide resolved
avalon/lib.py Outdated Show resolved Hide resolved
@davidlatwe
Copy link
Collaborator

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
Copy link
Collaborator Author

BigRoy commented Jan 2, 2020

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?

avalon/lib.py Outdated Show resolved Hide resolved
avalon/lib.py Outdated Show resolved Hide resolved
@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 2, 2020

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
Copy link
Collaborator Author

BigRoy commented Jan 2, 2020

This is fixed with: 241e05d

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

@jasperges
Copy link
Contributor

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

Copy link
Collaborator

@davidlatwe davidlatwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay.
I believe we could merge this, today ! 🚀

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 7, 2020

Merging this.

@BigRoy BigRoy merged commit 94aee92 into getavalon:master Jan 7, 2020
davidlatwe added a commit to davidlatwe/core that referenced this pull request Jan 7, 2020
davidlatwe added a commit to davidlatwe/core that referenced this pull request Jan 7, 2020
jasperges added a commit to jasperges/avalon-core that referenced this pull request Jan 8, 2020
@BigRoy BigRoy deleted the patch_499 branch January 25, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants