Refactor dependency management logic for consistency and separation from UI#7
Conversation
- Adds a method to the UpliftManager to keep track of the dependencies in your project, and what state they are in. - Use this method to improve consistency in the menu Check Dependencies - Use this method to improve consistency in the Update Window
| return dependenciesState.ToArray(); | ||
| } | ||
|
|
||
| public void AppendDependencyState( |
| Upbring upbring = Upbring.Instance(); | ||
| PackageRepo[] targets = GetTargets(GetDependencySolver(), InstallStrategy.UPDATE_LOCKFILE); | ||
|
|
||
| bool any_installed = |
| GUI.enabled = true; | ||
| if(!definition.Requirement.IsMetBy(installed.Version)) | ||
| EditorGUILayout.HelpBox( | ||
| "The version of the package currently installed does not match the requirements of your project!", |
There was a problem hiding this comment.
maybe display the version and the requirements?
There was a problem hiding this comment.
I believe that this is not necessary as it is displayed just above:
There was a problem hiding this comment.
Should we still display it?
| snapshot.installableDependencies[index].Package = package; | ||
| else | ||
| { | ||
| PackageRepo[] temp = snapshot.installableDependencies; |
There was a problem hiding this comment.
Would this work?
snapshot.installableDependencies = Array.Resize<PackageRepo[]>(snapshot.installableDependencies, snapshot.installableDependencies.Length + 1);
snapshot.installableDependencies[snapshot.installableDependencies.Length] = = new PackageRepo { Package = package };
| ); | ||
|
|
||
| if(state.transitive) | ||
| message = "`--> " + string.Join(" \n", message.Split('\n')); |
There was a problem hiding this comment.
Yes, so this looks a bit like an arrow going down. I think it looks good, but we can remove it if you want
There was a problem hiding this comment.
________$$$$
_______$$__$
_______$___$$
_______$___$$
_______$$___$$
________$____$$
________$$____$$$
_________$$_____$$
_________$$______$$
__________$_______$$
____$$$$$$$________$$
__$$$_______________$$$$$$
_$$____$$$$____________$$$
_$___$$$__$$$____________$$
_$$________$$$____________$
__$$____$$$$$$____________$
__$$$$$$$____$$___________$
__$$_______$$$$___________$
___$$$$$$$$$__$$_________$$
____$________$$$$_____$$$$
____$$____$$$$$$____$$$$$$
_____$$$$$$____$$__$$
_______$_____$$$_$$$
________$$$$$$$$$$
There was a problem hiding this comment.
This is a bit confusing though, do you approve, or do you think we should indeed remove it
| InstallPackages(targets); | ||
| } | ||
|
|
||
| public DependencyState[] GetDependenciesState(bool refresh = false) |
There was a problem hiding this comment.
Do we want to add the install strategy argument as in the above method's API? Adding it later would make them "out of order".
There was a problem hiding this comment.
I don't think so: we want to be sure that the install strategy is InstallStrategy.UPDATE_LOCKFILE to be sure that we get all the best targets that could update the packages.
| ref List<DependencyState> dependenciesState, | ||
| DependencyDefinition definition, | ||
| PackageRepo[] targets, | ||
| bool any_installed, |
| latest = PackageList.Instance().GetLatestPackage(definition.Name) | ||
| }; | ||
|
|
||
| state.bestMatch = targets.First(pr => pr.Package.PackageName == definition.Name); |
There was a problem hiding this comment.
Given that we just refreshed, could that fail? I.e. targets.First return null?
There was a problem hiding this comment.
I'm not sure. We go through the dependencies as does the targets, so there should not be an issue. We can still check for null though, but what behaviour should we have then?
niezbop
changed the title
2 participants
This makes sure that the dependency state is being kept tracked of in a consistent way, as well as keeping the lockfile udpated along update operations.
Improvements:
Extract logic from
Uplift > Check Dependenciesand theUpdateUtilitywindow to only keep display role, and move that logic to the UpliftManager inGetDependenciesState().Use the
LogAggregatorinUplift > Check Dependenciesso that it doesn't flood the console.Fixes:
Previously mentionned refactoring made sure that the logic is consistent with the project requirements and the Update window will no longer allow to update to conflicting requirements.
InstallPackage has been lightly modified to make sure that Update actions will keep the lockfile updated.
Both of these fixes address #5