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

Made packages.config path configurable from NuGet preferences #551

Merged
merged 10 commits into from
Aug 11, 2023

Conversation

popara96
Copy link
Collaborator

@popara96 popara96 commented Aug 4, 2023

Minor changes in order to make packages.config path configurable. We don't allow for the path to be outside of Assets folder since we still need OnPostprocessAllAssets to be triggered if this file is changed.

@popara96 popara96 requested review from JoC0de and igor84 August 4, 2023 09:36
Comment on lines 46 to 47
relativeTo = Path.GetFullPath(relativeTo);
path = Path.GetFullPath(path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should probably only accept full paths and not call GetFullPath on them. If someone passed relative paths to it this would not work correctly if called from CLI version since CurrentDirectory and project path set through the argument could be different. We could throw or return path if Path.IsPathRooted returned false for any of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The CLI can use Directory.SetCurrentDirectory(projectPath); to get the same behavior.

src/NuGetForUnity/Editor/PackagesConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/PackagesConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity.Tests/Assets/NuGet.config Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetPreferences.cs Outdated Show resolved Hide resolved
@JoC0de
Copy link
Collaborator

JoC0de commented Aug 6, 2023

@popara96 i will fix the CLI soon.

src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetPreferences.cs Outdated Show resolved Hide resolved
…fig file path is outside of Assets instead of logging
@popara96 popara96 requested review from igor84 and JoC0de August 9, 2023 12:31
src/NuGetForUnity/Editor/NugetPreferences.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetPreferences.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

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

Sorry for that much changes. Together we are getting a nice solution. 👍

src/NuGetForUnity/Editor/NugetConfigFile.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetHelper.cs Outdated Show resolved Hide resolved
src/NuGetForUnity/Editor/NugetPreferences.cs Outdated Show resolved Hide resolved
@popara96 popara96 requested a review from JoC0de August 10, 2023 09:50
Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

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

Now we can merge 👍

@popara96 popara96 merged commit 4493f3a into master Aug 11, 2023
8 checks passed
@popara96 popara96 deleted the configurable-config-path branch August 11, 2023 08:50
@popara96
Copy link
Collaborator Author

Thank you for the thorough review @JoC0de !

@JoC0de
Copy link
Collaborator

JoC0de commented Aug 11, 2023

Thank you for the improvement @popara96

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