feat: implement recursive placeholder resolution in Default parameters#1013
feat: implement recursive placeholder resolution in Default parameters#1013AshokThangavel wants to merge 10 commits intointersystems:mainfrom
Conversation
AshokThangavel
requested review from
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
A few comments and questions
| continue:data'["${" | ||
|
|
||
| set varExpr = "${" _ $piece($piece(data, "${", 2), "}") _ "}" | ||
| set resolved = ##class(%IPM.Utils.Module).%EvaluateSystemExpression(varExpr) |
There was a problem hiding this comment.
Does this not work if you pass the whole string into %EvaluateSystemExpression and let it resolve all the expressions at once instead of just extracting the first one?
There was a problem hiding this comment.
I've modified the logic to ensure the system variables and custom variables.
-Support nested ${var} and {$var} delimiters and handled Frankenstein-case
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
Thanks for the updates! Here's another pass of comments/questions
| } | ||
|
|
||
| /// Create class definition at runtime to capute the resovled reference value through <invoke> in manifest | ||
| ClassMethod CreateClassdef() As %Status |
There was a problem hiding this comment.
I'm not sure I quite understand. Could you please explain why we need a new dynamically created class to capture frankenstein when the other defaults don't?
There was a problem hiding this comment.
The reason I created this class was to verify that placeholders are correctly translated and passed to the method. Additionally I captured other variables in args... and temple global verify those variables. However, I explicitly check frankenstein for an additional layer of confirmation to ensure everything works as expected during testing. Thank you!
…xpression - Adopted %EvaluateSystemExpression for system variable resolution to align with existing APIs. - Retained multi-pass logic to handle nested and "Frankenstein" placeholders. - Implemented a priority gate: customParams take precedence over system defaults. - Hardened maxLevels condition using an explicit guard clause for better robustness.
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
Just a few more things, thanks!
| do $$$LogMessage("Validate the place holder variable resolved values in ^IRIS.Temp.IPMVarTest global") | ||
| set data = $get(^IRIS.Temp.IPMVarTest("frankenstein")) | ||
| if data'="" { | ||
| do $$$AssertTrue(1,"frankenstein place holder value is resolved to "_data) |
There was a problem hiding this comment.
You probably want to $$$AssertEquals() here instead to check it actually evaluates properly
There was a problem hiding this comment.
Got it. Added the $$$AssertEquals()
| for { | ||
| set sub = $order(^IRIS.Temp.IPMVarTest(sub),1,data) | ||
| quit:sub="" | ||
| do $$$LogMessage("^IRIS.Temp.IPMVarTest("_sub_") and value : "_data) |
There was a problem hiding this comment.
Since you're passing actual expressions into this method, you might as well test that they're also correct (and not just frankenstein)
There was a problem hiding this comment.
Thank you! I've updated the integration testing code and verified actual parameters values.
2 participants
Support Recursive Placeholder Resolution in Module Parameter
Description
This PR introduces deeply nested variables or chained configuration settings where one
${variable}depends on another.with a safety handle complex dependencies and prevent infinite loops in the case of circular references.
fixes: #1009
Changes
ResolvePlaceholdersto use awhileloop that continues as long as substitutions are being made.Testing Performed
${A}->${B}->${C}->FinalValue.Test.PM.Integration.Modulethat:module.xmlfrom XData.loadcommand to verify the full lifecycle of the variable resolution.Test Execution Results:
Test.PM.Integration.Module