Add new ComposingTransitionFactory class. #7877
Conversation
| Preconditions.checkNotNull(transition1); | ||
| Preconditions.checkNotNull(transition2); | ||
|
|
||
| if (transition1 == NullTransition.INSTANCE || transition1.isHostTransition()) { |
There was a problem hiding this comment.
Is there some way we can keep a single source of truth for isFinal()? i.e. what happens if isFinal() is changed for some TransitionFactory but this logic doesn't get updated?
Less strongly, can we at least keep the isFinal method in here rather than explicitly calling out NullTransition and HostTransition twice?
There was a problem hiding this comment.
There is currently no isFinal for ConfigurationTransition instances, so the only way to handle this is by explicit checks of specific transition types.
Maybe this is actually cleaner? I could remove isFinal from TransitionFactory and just check specific types in ComposingTransitionFactory instead. I think this is the only place where isFinal is used.
There was a problem hiding this comment.
I'm actually okay either way - keeping it in the TransitionFactory definition or centrally checking.
Whichever way though, let's avoid explicitly checking for NullTransition and HostTransition twice or more. For example, defining isFinal() in ComposingTransition and calling that form ComposingTransition.of and ComposingTransitionFactory.
If isFinal remains in TransitionFactory I'm quite sure how to access that source of truth in ComposingTransition.
There was a problem hiding this comment.
Removed isFinal from TransitionFactory, gave ComposingTransition and ComposingTransitionFactory better isFinal methods for checking.
| * Creates a {@link ComposingTransitionFactory} that applies the given factories in sequence: | ||
| * {@code fromOptions -> transition1 -> transition2 -> toOptions }. | ||
| * | ||
| * <p>Note that this method checks for transition factoriess that cannot be composed, such as if |
| if (NoTransition.isInstance(transitionFactory2)) { | ||
| // Since transitionFactory2 causes no changes, use transitionFactory1 directly. | ||
| return transitionFactory1; | ||
| } else if (NullTransition.isInstance(transitionFactory2) || transitionFactory2.isHost()) { |
There was a problem hiding this comment.
Similar to earlier, why not just check transitionFactory2.isFinal() here?
...om/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
Show resolved
Hide resolved
Part of #7814.