Adding zpm "ci" command to install from a lock file#1080
Adding zpm "ci" command to install from a lock file#1080isc-jlechtne wants to merge 11 commits intomainfrom
Conversation
isc-jlechtne
requested review from
isc-cborbonm,
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
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.
Good overall! Just a few small things. Also you'll want to add a description.
| } elseif (tCommandInfo = "history") { | ||
| do ..History(.tCommandInfo) | ||
| } elseif (tCommandInfo = "ci") { | ||
| do ..CleanInstall(.tCommandInfo) |
There was a problem hiding this comment.
It's a bit confusing to me to have "ci" stand for "CleanInstall", since 1) CI=continuous integration, and 2) it doesn't mention the lock file like "LockFileInstall" would for example.
But taking a step back, what's the benefit of having a new command versus using a flag on install/load?
There was a problem hiding this comment.
The initial plan was to have creation of a lock file be tied to install/load but installing from it its own method. However, the more I think about this, I do think you are correct. Functionally, I don't think there is anything that requires a whole new method as opposed to the new flag you suggested. I'll make the switch for the next PR iteration (and we can always back track if a reason comes up to).
As for the naming part: The reason we called it ci/CleanInstall for consistency since that is the same name used in other package managers.
| set verbose = $get(commandInfo("data","Verbose")) | ||
| set log = ##class(%IPM.General.HistoryTemp).CleanInstallInit(moduleName) | ||
|
|
||
| // TODO: Add "path"? (see Update() for more info of calling install v load) |
There was a problem hiding this comment.
When is this todo being done or removed?
There was a problem hiding this comment.
It is being done. This was just a reminder to myself when putting up the first commit and PR for comments that I still need to account for that case.
2 participants
No description provided.