Updating FileCopy to check for $ variables in path #660
Conversation
isc-jlechtne
requested review from
isc-tleavitt,
isc-kiyer,
isc-eneil,
isc-shuliu and
isc-jili
| @@ -39,7 +39,12 @@ Method OnBeforePhase(pPhase As %String, ByRef pParams) As %Status | |||
| } | |||
|
|
|||
| If (pPhase = "Activate") && (..InstallDirectory '= "") && ('..Defer) { | |||
| Set tSource = ..ResourceReference.Module.Root _ $Case(..SourceDirectory, "": ..ResourceReference.Name,: ..SourceDirectory) | |||
| if ($Extract(..ResourceReference.Attributes.GetAt("SourceDirectory"), 0, 2) = "{$") { | |||
There was a problem hiding this comment.
@isc-jlechtne Please add a comment to explain what's going on here for the sake of future me who will have forgotten.
| @@ -39,7 +39,12 @@ Method OnBeforePhase(pPhase As %String, ByRef pParams) As %Status | |||
| } | |||
|
|
|||
| If (pPhase = "Activate") && (..InstallDirectory '= "") && ('..Defer) { | |||
There was a problem hiding this comment.
@isc-jlechtne, it seems like everything in this if statement and the one below is identical. Perhaps that can be separated out into a helper method that cleans up the source directory, sets the target, and runs DoCopy(...) iff the phase is Activate and the Installer directory is not empty.
Or at least, break out the logic to clean up the source directory?
| @@ -121,7 +132,21 @@ Method NormalizeNames(ByRef pSource As %String, ByRef pTarget As %String, Output | |||
| } | |||
| } | |||
|
|
|||
| Method DoCopy(tSource, tTarget, pParams) | |||
| Method GetSource() | |||
There was a problem hiding this comment.
@isc-jlechtne
Nit: I'd name this GetNormalizedSourcePath.
Also, it's worth adding one line of method documentation to explain that this prefixes the path to the module root if no $ variables are used.
There was a problem hiding this comment.
@isc-eneil I don't want to name it Normalized since the normalization actually happens in the DoCopy method. My comment inside the method is kind of the inverse of what you're saying but changing it to a method description instead as you suggested does make sense.
There was a problem hiding this comment.
That's fair. I'm good with GetSource then.
No description provided.