-
Notifications
You must be signed in to change notification settings - Fork 655
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
Refactor caching system in GitVersion #3797
Conversation
Changes include updating cache file format from YML to JSON, removing unused methods, and optimizing file write/read operations. This allows to remove dependency on YamlDotNet. Various changes were also made to use more appropriate method identifiers and improve handling of cache file exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
@@ -1,7 +1,6 @@ | |||
using System.Text.Encodings.Web; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we shouldn't have a reference to System.Text.Encodings.Web
or anything else besides System
in GitVersion.Core
.
using System.Text.Encodings.Web; |
@@ -14,6 +13,22 @@ public static GitVersionVariables FromJson(string json) | |||
return FromDictionary(variablePairs); | |||
} | |||
|
|||
public static string ToJsonString(this GitVersionVariables gitVersionVariables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move the serialization of GitVersionVariables
out from GitVersion.Core
so we don't need dependencies on JSON? Ideally, I'd like to delete all <PackageReference>
in GitVersion.Core.csproj
.
GitVersion/src/GitVersion.Core/GitVersion.Core.csproj
Lines 14 to 19 in d3d16cc
<ItemGroup> | |
<PackageReference Include="Polly" /> | |
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> | |
<PackageReference Include="Microsoft.Extensions.Options" /> | |
<PackageReference Include="YamlDotNet" /> | |
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started gradually to remove the depenencies, or at least with those non microsoft first, hope we can get to a point when Core is dependency free*.
For now I want to get this one to a point when the build passes and then we can prepare a beta.4
Closing this one for now, will open a new PR from a feature branch, not forked |
Changes include updating cache file format from YML to JSON (breaking change), removing unused methods, and optimizing file write/read operations. This allows to remove dependency on YamlDotNet. Various changes were also made to use more appropriate method identifiers and improve handling of cache file exceptions.