Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Validate clashing sibling names when
strip namespaces
is enabled #112Validate clashing sibling names when
strip namespaces
is enabled #112Changes from 3 commits
177a434
a9d5ee1
93d8d8e
0865ec9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@iLLiCiTiT this really seems like madness - but I'm admittedly not entirely sure how to do this better. Many of these plugins can be enabled/disabled, they can be optional and hence disabled for the instance only, they can either have exposed publish attributes for
stripNamespaces
or have it locked down and defined by settings as defaults.Only way to simplify that is maybe moving this to a singular 'toggle' to a dedicated Collector which puts it in
instance.data
? But it'd not be backwards compatible.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 actually sounds more legit to me. Single plugin that is capable of showing all the input fields and based on the checked fields adds families (or different data) to instance. But that is a lot of work, so I don't know if should be handled in this PR.
OptionalPyblishPluginMixin
was not really meant that it is dependent on other plugins. BTW shouldn't the other plugin add something to instance data (for now) instead of having this logic dependent on current state of instance (might not represent state of instance at the moment the other plugin was processed)?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.
The other plugins are Extractors - they run after the validators and hence the data is not there yet, so I'd need to move those to a Collector instead - which means we get into the shitshow of backwards compatibilities, etc. :)
This logic here is the best I could think of to support it now without refactoring everything.
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 do agree "correct implementation" would require massive refactor to allow it. I'll let @antirotor to decide.