Shuffle compatibility improvements #5741
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM |
The key change here is that `__plug()` now uses a series of `__getitem__` calls rather than a single call to `descendant()`, giving the compatibility configs the ability to monkey-patch a name change in at any step. The other changes aren't strictly necessary, but I switched from storing plug paths as "dot.separated.strings" to `( "tuples", "of", "names" )` since that's the form we now need them in most commonly, and avoids a bunch of string manipulations for the more common operations. This leaves us doing string manipulations only for the less common error-handling code paths.
And add test demonstrating that expressions can still be made using the old names for plugs. This is necessary for an internal expression created by a node at Cinesite.
johnhaddon
force-pushed
the
shuffleCompatImprovementsAgain
branch
from
726c843 to
c06ded1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This improves backwards compatibility for the recent improvements to the Shuffle node. As the tests now demonstrate, not only can you load old files (we had test coverage for this already), but you can now also create new expressions via the API using the legacy plug names. This should help Cinesite get over the upgrade hump, since they have nodes which create such expressions internally in their constructors.
The only difference between this PR and #5739 is that I've rebased to fix the Changes.md conflict and am making the PR from a branch on the main repo, to get GafferArnold in the build artifacts. @danieldresser-ie, this is still a priority for review.