Make RuleTransformer fully recursive [#257] #421
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
RuleTransformer for the past eleven years or more has first recursed, then applied the rewrite rules as it backed out of the recursion. That prevented rewrite rules from acting on changes of previous rules unless they recursed themselves. The workaround has always been chain rule transformers instead of calling one rule transformer with all the rules. This change basically re-implements RuleTransformer as that workaround, and introduces a NestingTransformer which does the nesting. Clearly, if you have N rules you'll now recurse N times instead of once, though each rule is still only applied once for each element. On the other hand, a RewriteRule that recursed would incur in exponential times, and now they don't need to. The original behavior has no reason to be. It didn't prevent rules from seeing each other changes, nor was it particularly concerned with performance. With API changes coming on 2.0, I believe this is the right time to introduce this change.
|
Fixes #257 |
SethTisue
changed the title
|
is there someone watching this repo who could review this...? |
And there was a deafening silence... |
|
@SethTisue I rely on the tests. I've always found it hard to reason about the XML transformation code. |
SethTisue
approved these changes
Dec 5, 2020
|
It's been 18 days now. |
|
another backstop is the community build; I've included this change here: scala/community-build#1333 |
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.
RuleTransformer for the past eleven years or more has first recursed,
then applied the rewrite rules as it backed out of the recursion.
That prevented rewrite rules from acting on changes of previous rules
unless they recursed themselves.
The workaround has always been chain rule transformers instead of
calling one rule transformer with all the rules. This change basically
re-implements RuleTransformer as that workaround, and introduces
a NestingTransformer which does the nesting.
Clearly, if you have N rules you'll now recurse N times instead of
once, though each rule is still only applied once for each element.
On the other hand, a RewriteRule that recursed would incur in
exponential times, and now they don't need to.
The original behavior has no reason to be. It didn't prevent
rules from seeing each other changes, nor was it particularly
concerned with performance. With API changes coming on 2.0, I
believe this is the right time to introduce this change.