Closed KieranKaelin closed 1 year ago
Thanks for the refactoring, a few of my concerns have been addressed, but I would like to see it going even further. Some of the refactoring could be improved even more.
For example the test test_replace_nvl. It kind of took me a while to understand how the rule is applied to cause the root
variable to change inside the test. By only looking at check_node(&root, expect![[r#"..."#]]);
it's not immediately obvious, the flow feels a bit off.
One option I could think of is, instead of passing the input text to the rule.apply()
function, the previously parsed root
variable could be used here instead. The logic to find a rule & to apply it could move into its own helper function, something like:
fn apply_rule(rule: &impl RuleDefinition, root: &mut Root) -> Result<TextRange, RuleError> {
let ctx = DboAnalyzeContext::default();
let rules = rule.find_rules(root, &ctx)?;
let location = rules
.first()
.ok_or_else(|| RuleError::RuleNotFound(rule.short_desc()))?;
let rule_location = RuleLocation::from_node(root.syntax(), location.range);
rule.apply(
&location.node,
&rule_location,
&DboAnalyzeContext::default(),
)
}
This allows to associate the rule
to the root
syntax node, making it clearer to signal that both are connected. Otherwise, first the list of locations is determined, then a rule applied which modifies the root
indirectly. Note that I chose to mark the root
function parameter as mutable. It's not strictly necessary (due to interior mutability), but I feel it signals to the caller that this variable is modified. The helper function finds the first rule, then applies it. Currently tests need to re-fetch the rule locations in order to apply them again. Instead of passing in the location, the RuleDefinition
is provided to search a match for.
Furthermore a few of the checks seem redundant. Imho testing the root
via the check_node
function already tests that the right location is replaced, it seems superfluous to check the text range with start
& end
explicitely. Using another helper function to parse the Root
for a given input this test could be re-written as follows without losing its clarity.
#[test]
fn test_replace_nvl() {
const INPUT: &str = include_str!("../../tests/dql/nvl-coalesce.ora.sql");
let mut root = parse_root(INPUT);
let rule = ReplaceNvl;
apply_rule(&rule, &mut root).expect("Failed to apply rule");
check_node(
&root,
expect![[r#"
SELECT coalesce(NVL(dummy, dummy), 'John'), JOHN.NVL() from dual;
"#]],
);
apply_rule(&rule, &mut root).expect("Failed to apply rule");
check_node(
&root,
expect![[r#"
SELECT coalesce(coalesce(dummy, dummy), 'John'), JOHN.NVL() from dual;
"#]],
);
}
Cheers @justahero, I'll implement your suggestions!
Another good candidate to refactor is the do_apply
closure that applies a RuleHint
. As this is declared at least twice, it's probably makes sense to move that code into a helper function.
This closure has the similar property that it somehow modifies the transpiled
input variable & also updates the resulting metadata
from it. For example the test code
check_metadata_rule(&metadata.rules[0], "CYAR-0001", 1, 27..27, 1, 28, 1, 28);
metadata = do_apply(&metadata.rules[0]);
check_metadata_rule(&metadata.rules[0], "CYAR-0002", 1, 30..32, 2, 1, 2, 3);
metadata = do_apply(&metadata.rules[0]);
check_metadata_rule(&metadata.rules[0], "CYAR-0003", 1, 281..292, 9, 4, 9, 15);
metadata = do_apply(&metadata.rules[0]);
check_metadata_rule(&metadata.rules[0], "CYAR-0005", 2, 56..63, 4, 15, 4, 22);
metadata = do_apply(&metadata.rules[0]);
took me a minute to parse. I think what the tests generally check are that rules are applied, for example to a Procedure
, then applying a set of transformation / rules until the result is fully (or mostly) transformed as expected & no rules left available to apply.
The nature of applying rules is that every modification on the text will require updates to all offsets / ranges for other rules / hints, which means the rules have to be determined again, in order to fetch the updated locations / ranges. It may be nice to check which rules have been applied, but I'm not sure there's much gain to know the exact offsets / ranges. I think it's sufficient to check that the final result matches the expectation. As all rules have been tested individually in their files it might be redundant to call the check_metadata_rule
function to see where they were applied.
Closes #86