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

Do not wrap single active recipe & remove activateAll() #4349

Merged
merged 6 commits into from
Jul 24, 2024

Conversation

timtebeek
Copy link
Contributor

What's changed?

Remove activateAll() and do not wrap single active recipe into a new CompositeRecipe.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

The CompositeRecipe overrides a number of methods like the estimated time and data table descriptors. I think it's fine to skip this indirection, but an extra pair of eyes can't hurt.

/**
* A recipe that exists only to wrap other recipes.
* Anonymous recipe classes aren't serializable/deserializable so use this, or another named type, instead
*/
@RequiredArgsConstructor
public class CompositeRecipe extends Recipe {
private final List<Recipe> recipeList;
@Override
public String getDisplayName() {
return getName();
}
@Override
public String getDescription() {
return "A recipe that consists of a list of other recipes.";
}
@Override
public Duration getEstimatedEffortPerOccurrence() {
return null;
}
@Override
public List<Recipe> getRecipeList() {
return recipeList;
}
@Override
public List<DataTableDescriptor> getDataTableDescriptors() {
List<DataTableDescriptor> dataTableDescriptors = null;
for (Recipe recipe : getRecipeList()) {
List<DataTableDescriptor> dtds = recipe.getDataTableDescriptors();
if (!dtds.isEmpty()) {
if (dataTableDescriptors == null) {
dataTableDescriptors = new ArrayList<>();
}
for (DataTableDescriptor dtd : dtds) {
if (!dataTableDescriptors.contains(dtd)) {
dataTableDescriptors.add(dtd);
}
}
}
}
return dataTableDescriptors == null ? super.getDataTableDescriptors() : dataTableDescriptors;
}
}

Have you considered any alternatives or workarounds?

We can leave this indirection in for consistent handling no matter the number of active recipes.

Any additional context

@timtebeek timtebeek added the enhancement New feature or request label Jul 22, 2024
@timtebeek timtebeek requested a review from sambsnyd July 22, 2024 21:58
@timtebeek timtebeek self-assigned this Jul 22, 2024
@timtebeek timtebeek requested a review from pstreef July 23, 2024 11:56
github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@timtebeek
Copy link
Contributor Author

@pstreef it seems like this indirection was masking missing descriptions in our test recipes; are you still ok with this updated PR?
I imagine we'll see this issue pop up some more downstream, although it seems like we already had similar unpacking on the Moderne side of things.

@timtebeek timtebeek merged commit 02914e3 into main Jul 24, 2024
2 checks passed
@timtebeek timtebeek deleted the do-not-wrap-single-active-recipe branch July 24, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants