Align estimated time savings between Result and SourcesFileResults
#6022
Conversation
This PR solves 2 issues: 1. Result only returns the estimated time savings from the first recipe that made a change to a file. This change sums up the estimated time of all changes to a file (however, this still does not take occurrences into account). 2. `SourcesFileResults` has rows for all recipes in the hierarchy for tracability. Because each of these rows has an estimated time savings (of the actual change) summing the column gives the wrong values. By only setting the value to the leaf recipe this column becomes sum-able.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Outdated
Show resolved
Hide resolved
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Outdated
Show resolved
Hide resolved
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Outdated
Show resolved
Hide resolved
| @@ -68,8 +73,7 @@ public Result(@Nullable SourceFile before, @Nullable SourceFile after, Collectio | |||
| if (recipesStack != null && !recipesStack.isEmpty()) { | |||
| Duration perOccurrence = recipesStack.get(recipesStack.size() - 1).getEstimatedEffortPerOccurrence(); | |||
| if (perOccurrence != null) { | |||
| timeSavings = perOccurrence; | |||
| break; | |||
| timeSavings = timeSavings.plus(perOccurrence); | |||
There was a problem hiding this comment.
This fundamentally alters the intention of the calculation from only summing one of the recipes in the stack to each recipe in the stack contributing time savings. I think we intentionally did not design it this way.
There was a problem hiding this comment.
Exactly, which is why I noted there might be a good reason for this in the description. Either way not summing these here is what introduces the difference in what SourcesFileResult summing (if done correctly) gives you. This is confusing people.
I think neither approach is "fully correct" because neither takes occurences into account, however, there currently isn't a viable mechanism to do that.
I wonder if so, why it was designed this way. Why are we picking the first? And nog the sum or the highest value?
There was a problem hiding this comment.
Now re-reading what you said. We're not summing the recipes in 1 stack, but all the last (leaf) recipes in each stack. So that if multiple recipe (stacks) made a change to a file, we sum those estimated times up. Just like we would in the (corrected) SourcesFileResults
github-project-automation
bot
moved this from Ready to Review
to In Progress
in OpenRewrite
| @@ -138,6 +142,10 @@ private static boolean subtreeChanged(Tree root, Map<UUID, Tree> beforeTrees) { | |||
| }.reduce(root, new AtomicBoolean(false)).get(); | |||
| } | |||
|
|
|||
| /** Sum of estimated time savings of all recipes that made changes to this source file. | |||
There was a problem hiding this comment.
nit: incorrectly formatted Javadoc.
| @@ -129,6 +131,99 @@ void customEstimatedEffortForRecipeThatGeneratesSourceFiles() { | |||
| )); | |||
| } | |||
|
|
|||
| @Test | |||
| void summedEstimatedEffortForMultipleChangesToSameFile() { | |||
There was a problem hiding this comment.
nit: this test at nearly 100 lines is too complicated to really understand at a glance.
|
It sounds like somewhere along the line |
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Show resolved
Hide resolved
Ok, so to solve the issue there we could remove the recursion altogether which adds the full stack. That solves 1 issue, but not the other. Do we want to keep the difference in the 2 summed estimated values? |
… to the logic in `Result.java`
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Show resolved
Hide resolved
rewrite-core/src/test/java/org/openrewrite/RecipeEstimatedEffortTest.java
Show resolved
Hide resolved
What's changed?
This PR solves 2 issues:
Result only returns the estimated time savings from the first recipe that made a change to a file. This change sums up the estimated time of all changes to a file (however, this still does not take occurrences into account).
SourcesFileResultshas rows for all recipes in the hierarchy for tracability. Because each of these rows has an estimated time savings (of the actual change) summing the column gives the wrong values. By only setting the value to the leaf recipe this column becomes sum-able.What's your motivation?
Aligning the estimated time savings and making the results easier to understand
Anything in particular you'd like reviewers to focus on?
There might have been a good reason to only expose the first value in
Result.Anyone you would like to review specifically?
@jkschneider
Have you considered any alternatives or workarounds?
We could chose to only go forward with either solution, however I expect this will remain hard to explain.
Note: that this solution works around the problem that we do not have a number of occurrences per Recipe (stack) in Result . If we would have that occurrence count (which would be a non trivial change) we could give an actual value and not just assume 1 change by each recipe that made a change.
Any additional context
Checklist