3 ms·
> I did consider two other approaches: Changing Ruleset from being []Rule to []*Rule, which would mean we no longer need to explicitly take a reference to th
by karmakaze 4y ago
> I did consider two other approaches:
Changing Ruleset from being []Rule to []*Rule, which would mean we no longer need to explicitly take a reference to the rule.
Returning a Rule rather than a *Rule. This would still copy the Rule, but it should stay on the stack instead of moving to the heap.
> However, both of these would have resulted in a breaking change as this method is part of the public API.
The problem with heap allocated objects could be due to the incorrect public API.
The change that improves performance also gives out pointers to the actual elements of Ruleset itself permitting the caller to change the contents of Ruleset which wasn't possible before the speed-up. Perhaps you're already aware since change to []*Rule was being considered.
- ok_dad 4y agoSometimes it doesn't matter if a public API is incorrect, because it's set in stone for whatever reason, and you just need to fix the problem internally.
- spockz 4y agoThis is why I like https://github.com/openrewrite https://github.com/openrewrite so much. One gets to tell users how to rewrite code automatically. It makes refactoring almost as easy as in a mono repo.
- bspammer 4y agoThis looks super interesting, but as far as I can tell they don’t go into how to inform downstream users to add your recipes to their maven/gradle configuration when you make a breaking change. In my head, the ideal flow would be an annotation on your library function/class which triggers an IntelliJ suggestion for downstream users affected by your breaking change to run the automated refactor for them. Kinda like a more helpful @Deprecated.
- spockz 4y agoIt would definitely be nice to have this generated from a deprecated like annotation! The maven plug-in seems to use the recipes directly as dependencies: https://github.com/openrewrite/rewrite-maven-plugin https://github.com/openrewrite/rewrite-maven-plugin In our internal situation the parent Pom could already control this plug-in definition including versions of the recipes. At the very least the recipes could follow the same version as the artefact. I thought I saw somewhere where the recipe was bundled with the artefact itself. That is very neat for simple usecase. However, it suffers from the same flaw as the native-image configuration for GraalVM in artifacts. Sometimes the configuration/recipe needs to change. Eg because of new insights or because it is incompatible with new versions of GraalVM/open rewrite. So I suppose having an extra version dimension Could solve this. Then one can always depend on the latest version of the recipe for that artifact.
- karmakaze 4y agoThe way to fix it in that manner here is to undo the 42% speedup and return the heap allocated object for the caller to mangle.
- mook 4y agoThe best part is, there's a subtle API breakage here: the returned *Rule is now a reference to an element of the []Rule, so if the caller was previously modifying the returned value it'll change the slice. It's debatable what API guarantees existed around this though; most of the time this would be unspecified.