Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split NugetHelper code into separate classes #560

Merged
merged 11 commits into from
Aug 30, 2023

Conversation

JoC0de
Copy link
Collaborator

@JoC0de JoC0de commented Aug 20, 2023

Moved code from NugetHelper

  • NuGet.config management (loading / active source) -> ConfigurationManager
  • Delete / move / copy folder functions -> FileSystemHelper
  • management of currently installed packages including managing the packages.config -> InstalledPackagesManager
  • Receiving nuget packages from cache / from the installed-packages -> NugetCacheManager
  • nuget.exe calls for push action -> NugetCliHelper
  • verbose logging -> NugetLogger
  • moving / deleting files after .nupkg file is extracted -> NugetPackageContentManager
  • installing nuget packages -> NugetPackageInstaller
  • restoring all nuget packages -> NugetPackageRestorer
  • uninstalling nuget packages -> NugetPackageUninstaller
  • updating nuget packages -> NugetPackageUpdater
  • handling unity project paths -> UnityPathHelper
  • handling web requests -> WebRequestHelper

There is still code that needs to be refactored but I wanted to start creating a structure so when we e.g. split the Install method that still has 178 lines of code the code has a 'place' and is not hidden in a 1600 line NugetHelper. Eventually we should move some files into folders. Im also not sure about the naming currently most files/classes have a Nuget-prefix eventually we can remove some of them e.g. NugetPackageInstaller vs PackageInstaller.
@igor84 what do you thing about this.

@JoC0de JoC0de requested review from igor84 and popara96 August 20, 2023 21:41
@JoC0de JoC0de self-assigned this Aug 20, 2023
@igor84
Copy link
Collaborator

igor84 commented Aug 21, 2023

I think it makes sense. This is and will most probably stay a project that only handles nuget packages so repeating Nuget in every class name is redundant.

Copy link
Collaborator

@igor84 igor84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. Interesting that you embraced procedural style (with all the static classes) :).

src/NuGetForUnity/Editor/ConfigurationManager.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/CredentialProviderHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/FileSystemHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/FileSystemHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/FileSystemHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/FileSystemHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/FileSystemHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/InstalledPackagesManager.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetPackageTextureHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/UnityPathHelper.cs Outdated Show resolved Hide resolved
@JoC0de
Copy link
Collaborator Author

JoC0de commented Aug 26, 2023

Nicely done. Interesting that you embraced procedural style (with all the static classes) :).

I thought about creating classes but I couldn't find a good separation, and as Unity always reloads the AppDomain when we install / uninstall a package the classes couldn't live long enough to get a advantage. So currently the static classes manage there state by themselves and 'Lazy' load all required information from the file system. Normally I am strongly against static fields or properties but as NuGetForUnity "lives" inside the Unity Editor and therefore has no possible Multi-Threading / scoping issues that come with static-lifetime I found it acceptable.

@igor84
I now also moved the files into a "basic" folder / namespace structure. What do you thing about the folder names? I struggled with the name for the NugetPackage classes: either Models (mostly used for Database models / or in MVC code) or alternatively more generally Data?

@igor84
Copy link
Collaborator

igor84 commented Aug 26, 2023

I agree with your reasoning. It makes sense in this case.

From where I'm standing Models sounds good. They are models in the context of this project. But Data is ok as well.

@JoC0de JoC0de merged commit 46608af into GlitchEnzo:master Aug 30, 2023
8 checks passed
@JoC0de JoC0de deleted the feature/cleanup-nuget-helper branch August 30, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants