-
Notifications
You must be signed in to change notification settings - Fork 48
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
Refactor avalon.tools to avoid cross-referencing imports between tools #440
Conversation
- Remove AssetView class which solely set 3 properties on the TreeView - Refactor `Node` to `Item` as it is for QAbstractItemModels - Refactor some variables to `snake_case` as opposed to `lowerCamelCase` - Add some todo notes (notes to self for cleanup)
- Plus clarify that the group/family config data is a cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start !
avalon/tools/widgets.py
Outdated
preserve_expanded_rows, | ||
preserve_selection, | ||
iter_model_rows | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we are not suppose to directly import functions and classes now ? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks David. Is that based on the e3 example in CONTRIBUTING.md
? I interpreted it as not importing as a shorter name and not necessarily removing all functions imports. Especially now that there's a lib
at multiple levels (avalon-wide + tools-wide + per tool) seeing lib.
wouldn't tell much.
But probably @mottosso can best describe his intentions. I'm fine with both, do note however that if this needs to be adapted that it accounts for more things, e.g. see these lines.
I did now see there's an import there that states:
from ..vendor import qtawesome as awesome
Which I believe should be:
from ..vendor import qtawesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That irritates me for a long time I think it should be from ..vendor import qtawesome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from ..vendor import qtawesome as awesome
This has now been removed and resolved with this commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that based on the e3 example in CONTRIBUTING.md?
I was actually referencing from A8 in CONTRIBUTING.md
, which aim to preserve the namespace :P
Especially now that there's a lib at multiple levels (avalon-wide + tools-wide + per tool) seeing lib. wouldn't tell much.
Yeah.. maybe this will work ? Say we are in bad luck, used all three levels' lib.py
in one module:
from ... import lib as avalon_lib
from .. import lib as tool_lib
from . import lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one's a hard problem.
On the one hand, word and line length obscure code. One the other, namespaces give the reader a hint at what a function or class is coming from.
For some modules, it's obvious.
qtawesome.some_function() # I know you're third-party
For others, not so much.
model = SomeModel() # Third-party, local module, separate root-level module?
Let's try this as a general guideline; preserve the leaf-level module namespace.
from .vendor import qtawesome
is Goodfrom .vendor.qtawesome import icon
is Bad
maybe this will work ?
This is another hard problem; on the one hand, we should use lib
at various levels, as they are context sensitive. A lib
under tools
is tools libraries, whereas a lib
under maya/
is Maya libraries. And on the other they complicate the above general guideline.
I think this is as elegant of a solution as we'll get, if we are to have at least some guideline for how to deal with it. Preserve the minimum number of namespaces; in this case, we need both tool
and lib
, if avalon_lib
is also present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mottosso - just to be complete, if I understand you correctly the above example to import as tool_lib
, etc. when required is considered the correctly approved change. Yes? 👍
With this last commit I checked out the pipeline features in Maya and ran through:
This now all worked so I consider this testable for others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more comments for discussion :)
4c30973
to
ae70d90
Compare
Ah, backward compatibility, but do we need to do that for this kind of low level refactoring ? I have some script/tools that, for example, used |
Exactly, as those have never been exposed to any public facing API these changes are in the realm of "allowed to change". So, those don't need to be backwards compatible and you'll have to refactor in-house code that used it to change along. We had some internal tools referencing things too. ;) Anyway, anything not exposed in the API is allowed to change without being fully backwards compatible. (e.g. a function name may behave differently, be renamed, moved, etc.). And the recommendation is not to reference those methods, but of course... you can if you really need to at your own risk, like now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've taken a closer look and I think it looks good.
Based on your description, I've deliberately not commented on the actual content or behavior of code (there is some debt in there), and treat this PR as only moving code around. If that's the case, then structurally I think this reads very well.
I suppose that "Approved" should have been "Requesting changed", as I think we should address my comments before merging. I'll let @davidlatwe hit the Merge button for this PR on my behalf. Great work guys. |
This should be ready to go if I understood @mottosso correctly here then there are now no open comments anymore as I refactored the I'd say, give it one other test run and feel free to merge @davidlatwe 🚀 Thanks all! |
avalon/tools/loader/app.py
Outdated
refresh_family_config, | ||
refresh_group_config, | ||
get_active_group_config, | ||
from ..lib import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this should be from .. import lib
which you're already doing above, but then not using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being imported three times (!) in three different ways, let's clean this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mottosso - well spotted! I tried to simplify and clean things up with this commit. Let me know if that suffices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray !!
If no other objections, will merge this tomorrow !
Ah, I believe we should bump the version to |
@davidlatwe this hasn't been merged yet. Do you want me to quickly bump version? :) |
Yes, please do ! It seems that I could not bump it myself into this PR :P |
Note: This is a big PR as it refactors locations of models, views, delegates and functions. It's this big because unfortunately it needs to be.
Problems
Node
, which is wrong terminology as Qt refers toQAbstractItemModel
where it is anItem
loader.lib
are actually caches reused across multiple tools. This is unclear from the variable and function naming. (This "global state" is a problem of its own, but this PR will not try to solve that as it would require even more changes.) It only solves the cross-referencing across tools.lowerCamelCase
where they should besnake_case
as per Add CONVENTION.md #437AssetView
class exists which purely exists for one-time use and only sets 3 small view tweaks - it's redundant complexity.TreeModel
defines an attribute calledCOLUMNS
which should be lowercasecolumns
.ExactMatchesFilterProxyModel
which is redundant lines of code.What's changed?
avalon.tools.models
,avalon.tools.views
,avalon.tools.delegates
,avalon.tools.widgets
andavalon.tools.lib
Node
is refactored toItem
, including variablesnode
and alike that I could find are refactored toitem
.snake_case
.AssetView
class has been removed and the related lines of code can now directly be found in theAssetWidget
.TreeModel.COLUMNS
refactored toTreeModel.Columns
ExactMatchesFilterProxyModel
.This is a work in progress PR and has not been tested yet. However I'm opening this to kickstart the discussion about any faulty choices I'm making. Plus, since it's a lengthy PR it gives others time to go through what is being changed.
I'll clearly state in a comment when I have been able to thoroughly test changes and feel it's in a good state for others to test too.
This PR acts as a replacement to #414 trying to do some similar things yet now along the new Contribution guidelines. Additionally it tries to describe the problems that we faced before and how they have been resolved.