Release 0.8.1#14
Conversation
Add the auto execute process to fill up the version service data based on the git information
WalkthroughThis pull request introduces new classes and methods to enhance the Git process handling and versioning in Unity projects. It also extends the Changes
TipsChat with CodeRabbit Bot (
|
|
WalkthroughThe changes include the addition of two new classes: Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (3)
- Editor/GitEditorProcess.cs (1)
- Editor/VersionEditorUtils.cs (1)
- Runtime/Installer.cs (4)
Files ignored due to filter (4)
- Editor.meta
- Editor/GitEditorProcess.cs.meta
- Editor/VersionEditorUtils.cs.meta
- package.json
Files skipped from review due to trivial changes (1)
- Runtime/Installer.cs
Review comments generated (15)
- Review: 14
- LGTM: 1
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| using System; | ||
| using System.Diagnostics; | ||
|
|
||
| namespace GameLovers.Services.Editor |
There was a problem hiding this comment.
The GameLovers.Services.Editor namespace seems to be unrelated to the purpose of this class. Consider moving the GitEditorProcess class to a more appropriate namespace, such as GameLovers.Utils or GameLovers.VersionControl.
| /// <summary> | ||
| /// Run git commands processes that would otherwise be used in the terminal. | ||
| /// </summary> | ||
| /// <author> | ||
| /// https://blog.somewhatabstract.com/2015/06/22/getting-information-about-your-git-repository-with-c/ | ||
| /// </author> | ||
| public class GitEditorProcess : IDisposable |
There was a problem hiding this comment.
Consider adding XML documentation comments to provide descriptions for the class and its members.
| private const string DefaultPathToGitBinary = "git"; | ||
|
|
There was a problem hiding this comment.
The DefaultPathToGitBinary constant is not used in the code. If it's intended to be used as a default value for the pathToGitBinary parameter, consider removing it since the default value can be specified directly in the parameter declaration.
| public GitEditorProcess(string workingDir, string pathToGitBinary = DefaultPathToGitBinary) | ||
| { | ||
| var startInfo = new ProcessStartInfo | ||
| { | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| FileName = pathToGitBinary, | ||
| CreateNoWindow = true, | ||
| WorkingDirectory = workingDir | ||
| }; |
There was a problem hiding this comment.
Consider using the var keyword for the startInfo variable to improve readability.
34:
Consider using object initializer syntax to initialize the Process field instead of assigning properties individually.
| /// <summary> | ||
| /// Is this unity project a git repo? | ||
| /// </summary> | ||
| public bool IsValidRepo() | ||
| { | ||
| return ExecuteCommand("rev-parse --is-inside-work-tree") == "true"; | ||
| } | ||
|
|
There was a problem hiding this comment.
The IsValidRepo method could be simplified by directly returning the result of the ExecuteCommand method call.
| { | ||
| private const int ShortenedCommitLength = 8; | ||
|
|
There was a problem hiding this comment.
The ShortenedCommitLength constant is not used in the code. It can be removed.
- private const int ShortenedCommitLength = 8;| public static void SetAndSaveInternalVersion(bool isStoreBuild) | ||
| { | ||
| var newVersionData = GenerateInternalVersionSuffix(isStoreBuild); | ||
| var newVersionDataSerialized = JsonUtility.ToJson(newVersionData); | ||
| var oldVersionDataSerialized = LoadVersionDataSerializedSync(); | ||
| if (newVersionDataSerialized.Equals(oldVersionDataSerialized, StringComparison.Ordinal)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Debug.Log($"Saving new version data: {newVersionDataSerialized}"); | ||
| SaveVersionData(newVersionDataSerialized); | ||
| } |
There was a problem hiding this comment.
The SetAndSaveInternalVersion method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:
- Extract the logic for generating the new version data into a separate method.
- Move the comparison of old and new version data to a separate method.
- Use early returns instead of nested if statements to simplify the control flow.
Here's an example of how the refactored method could look like:
public static void SetAndSaveInternalVersion(bool isStoreBuild)
{
var newVersionData = GenerateInternalVersionSuffix(isStoreBuild);
var newVersionDataSerialized = JsonUtility.ToJson(newVersionData);
var oldVersionDataSerialized = LoadVersionDataSerializedSync();
if (IsVersionDataEqual(newVersionDataSerialized, oldVersionDataSerialized))
{
return;
}
Debug.Log($"Saving new version data: {newVersionDataSerialized}");
SaveVersionData(newVersionDataSerialized);
}
private static bool IsVersionDataEqual(string versionData1, string versionData2)
{
return versionData1.Equals(versionData2, StringComparison.Ordinal);
}| public static string LoadVersionDataSerializedSync() | ||
| { | ||
| var textAsset = Resources.Load<TextAsset>(VersionServices.VersionDataFilename); | ||
| if (!textAsset) | ||
| { | ||
| Debug.LogError("Could not load internal version from Resources."); | ||
| return string.Empty; | ||
| } | ||
|
|
||
| var serialized = textAsset.text; | ||
| Resources.UnloadAsset(textAsset); | ||
| return serialized; |
There was a problem hiding this comment.
The LoadVersionDataSerializedSync method loads the game version from a text asset in the Resources folder. However, it does not handle exceptions that may occur during the loading process. Consider adding proper error handling and logging in case the loading fails.
Here's an example of how the method could be modified:
public static string LoadVersionDataSerializedSync()
{
try
{
var textAsset = Resources.Load<TextAsset>(VersionServices.VersionDataFilename);
if (textAsset != null)
{
var serialized = textAsset.text;
Resources.UnloadAsset(textAsset);
return serialized;
}
}
catch (Exception e)
{
Debug.LogException(e);
}
Debug.LogError("Could not load internal version from Resources.");
return string.Empty;
}| private static VersionServices.VersionData GenerateInternalVersionSuffix(bool isStoreBuild) | ||
| { | ||
| var data = new VersionServices.VersionData(); | ||
|
|
||
| using (var repo = new GitEditorProcess(Application.dataPath)) | ||
| { | ||
| try | ||
| { | ||
| if (!repo.IsValidRepo()) | ||
| { | ||
| Debug.LogWarning("Project is not a git repo. Internal version not set."); | ||
| } | ||
| else | ||
| { | ||
| var branch = repo.GetBranch(); | ||
| if (string.IsNullOrEmpty(branch)) | ||
| { | ||
| Debug.LogWarning("Could not get git branch for internal version"); | ||
| } | ||
| else | ||
| { | ||
| data.BranchName = branch; | ||
| } | ||
|
|
||
| var commitHash = repo.GetCommitHash(); | ||
| if (string.IsNullOrEmpty(commitHash)) | ||
| { | ||
| Debug.LogWarning("Could not get git commit for internal version"); | ||
| } | ||
| else | ||
| { | ||
| data.Commit = commitHash.Substring(0, ShortenedCommitLength); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Debug.LogException(e); | ||
| Debug.LogWarning("Could not execute git commands. Internal version not set."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The GenerateInternalVersionSuffix method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:
- Extract the logic for retrieving git branch and commit into separate methods.
- Use early returns instead of nested if statements to simplify the control flow.
Here's an example of how the refactored method could look like:
private static VersionServices.VersionData GenerateInternalVersionSuffix(bool isStoreBuild)
{
var data = new VersionServices.VersionData();
using (var repo = new GitEditorProcess(Application.dataPath))
{
try
{
if (!repo.IsValidRepo())
{
Debug.LogWarning("Project is not a git repo. Internal version not set.");
return data;
}
var branch = GetGitBranch(repo);
if (string.IsNullOrEmpty(branch))
{
Debug.LogWarning("Could not get git branch for internal version");
}
else
{
data.BranchName = branch;
}
var commitHash = GetGitCommitHash(repo);
if (string.IsNullOrEmpty(commitHash))
{
Debug.LogWarning("Could not get git commit for internal version");
}
else
{
data.Commit = commitHash.Substring(0, 8);
}
}
catch (Exception e)
{
Debug.LogException(e);
Debug.LogWarning("Could not execute git commands. Internal version not set.");
}
}
data.BuildNumber = PlayerSettings.iOS.buildNumber;
data.BuildType = isStoreBuild ? "prod" : "dev";
return data;
}
private static string GetGitBranch(GitEditorProcess repo)
{
return repo.GetBranch();
}
private static string GetGitCommitHash(GitEditorProcess repo)
{
return repo.GetCommitHash();
}| data.BuildNumber = PlayerSettings.iOS.buildNumber; | ||
| data.BuildType = isStoreBuild ? "prod" : "dev"; | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Set the internal version of this application and save it in resources. This should be | ||
| /// called at edit/build time. | ||
| /// </summary> | ||
| private static void SaveVersionData(string serializedData) | ||
| { | ||
| const string assets = "Assets"; | ||
| const string resources = "Build/Resources"; | ||
|
|
||
| var absDirPath = Path.Combine(Application.dataPath, resources); | ||
| if (!Directory.Exists(absDirPath)) | ||
| { | ||
| Directory.CreateDirectory(absDirPath); | ||
| } | ||
|
|
||
| // delete old file with incorrect extension | ||
| const string assetExtension = ".asset"; | ||
| var absFilePath = Path.Combine(absDirPath, VersionServices.VersionDataFilename); | ||
| if (File.Exists(Path.ChangeExtension(absFilePath, assetExtension))) | ||
| { | ||
| AssetDatabase.DeleteAsset( | ||
| Path.Combine(assets, resources, | ||
| Path.ChangeExtension(VersionServices.VersionDataFilename, assetExtension))); | ||
| } | ||
|
|
||
| // create new text file | ||
| const string textExtension = ".txt"; | ||
| File.WriteAllText(Path.ChangeExtension(absFilePath, textExtension), serializedData); | ||
|
|
||
| AssetDatabase.ImportAsset( | ||
| Path.Combine(assets, resources, | ||
| Path.ChangeExtension(VersionServices.VersionDataFilename, textExtension))); | ||
| } |
There was a problem hiding this comment.
The SaveVersionData method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:
- Use
Path.Combineto concatenate file paths instead of manual string concatenation. - Use
File.WriteAllTextto simplify writing the serialized data to a text file. - Use
AssetDatabase.ImportAssetto import the newly created text file into the project.
Here's an example of how the refactored method could look like:
private static void SaveVersionData(string serializedData)
{
const string assets = "Assets";
const string resources = "Build/Resources";
const string versionDataFilename = VersionServices.VersionDataFilename;
var absDirPath = Path.Combine(Application.dataPath, resources);
if (!Directory.Exists(absDirPath))
{
Directory.CreateDirectory(absDirPath);
}
// Delete old file with incorrect extension
const string assetExtension = ".asset";
var absFilePath = Path.Combine(absDirPath, versionDataFilename);
if (File.Exists(Path.ChangeExtension(absFilePath, assetExtension)))
{
AssetDatabase.DeleteAsset(Path.Combine(assets, resources, Path.ChangeExtension(versionDataFilename, assetExtension)));
}
// Create new text file
const string textExtension = ".txt";
File.WriteAllText(Path.ChangeExtension(absFilePath, textExtension), serializedData);
AssetDatabase.ImportAsset(Path.Combine(assets, resources, Path.ChangeExtension(versionDataFilename, textExtension)));
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 15
Files ignored due to filter (1)
- package.json
Files selected for processing (6)
- Editor.meta (1 hunks)
- Editor/GitEditorProcess.cs (1 hunks)
- Editor/GitEditorProcess.cs.meta (1 hunks)
- Editor/VersionEditorUtils.cs (1 hunks)
- Editor/VersionEditorUtils.cs.meta (1 hunks)
- Runtime/Installer.cs (4 hunks)
Files skipped from review due to trivial changes (3)
- Editor.meta
- Editor/GitEditorProcess.cs.meta
- Editor/VersionEditorUtils.cs.meta
| { | ||
| private const string DefaultPathToGitBinary = "git"; | ||
|
|
||
| private readonly Process Process; |
There was a problem hiding this comment.
The Process field is not encapsulated properly. It's a good practice to make fields private and provide public properties or methods for accessing them if necessary. This helps in maintaining the integrity of the data by preventing external code from changing it unexpectedly.
- private readonly Process Process;
+ private readonly Process _process;| public GitEditorProcess(string workingDir, string pathToGitBinary = DefaultPathToGitBinary) | ||
| { | ||
| var startInfo = new ProcessStartInfo | ||
| { | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| FileName = pathToGitBinary, | ||
| CreateNoWindow = true, | ||
| WorkingDirectory = workingDir | ||
| }; | ||
|
|
||
| Process = new Process { StartInfo = startInfo }; | ||
| } |
There was a problem hiding this comment.
The constructor of GitEditorProcess creates a new Process object but does not start it. If an exception occurs during the creation of the Process object, it might lead to resource leaks as the Dispose method will not be called. Consider using a factory method to create the Process object and handle any exceptions that might occur during its creation.
| Process.StartInfo.Arguments = args; | ||
| Process.Start(); | ||
| var output = Process.StandardOutput.ReadToEnd().Trim(); | ||
| Process.WaitForExit(); | ||
| return output; |
There was a problem hiding this comment.
The ExecuteCommand method starts a process, reads its output, and waits for it to exit. However, it does not check whether the process has exited successfully or not. If the process exits with a non-zero status code, this could indicate an error. Consider checking the exit code of the process and handling any errors appropriately.
- Process.Start();
- var output = Process.StandardOutput.ReadToEnd().Trim();
- Process.WaitForExit();
+ _process.StartInfo.Arguments = args;
+ _process.Start();
+ var output = _process.StandardOutput.ReadToEnd().Trim();
+ _process.WaitForExit();
+ if (_process.ExitCode != 0)
+ {
+ throw new InvalidOperationException($"Git command '{args}' failed with exit code {_process.ExitCode}");
+ }| public static void SetAndSaveInternalVersion(bool isStoreBuild) | ||
| { | ||
| var newVersionData = GenerateInternalVersionSuffix(isStoreBuild); | ||
| var newVersionDataSerialized = JsonUtility.ToJson(newVersionData); | ||
| var oldVersionDataSerialized = LoadVersionDataSerializedSync(); | ||
| if (newVersionDataSerialized.Equals(oldVersionDataSerialized, StringComparison.Ordinal)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Debug.Log($"Saving new version data: {newVersionDataSerialized}"); | ||
| SaveVersionData(newVersionDataSerialized); | ||
| } |
There was a problem hiding this comment.
The method SetAndSaveInternalVersion generates a new version data and compares it with the old one. If they are equal, it returns without saving the new version data. This could lead to situations where the version data is not updated even though other changes have been made in the codebase. Consider always saving the new version data regardless of whether it matches the old one.
- if (newVersionDataSerialized.Equals(oldVersionDataSerialized, StringComparison.Ordinal))
- {
- return;
- }| public static string LoadVersionDataSerializedSync() | ||
| { | ||
| var textAsset = Resources.Load<TextAsset>(VersionServices.VersionDataFilename); | ||
| if (!textAsset) | ||
| { | ||
| Debug.LogError("Could not load internal version from Resources."); | ||
| return string.Empty; | ||
| } | ||
|
|
||
| var serialized = textAsset.text; | ||
| Resources.UnloadAsset(textAsset); | ||
| return serialized; | ||
| } |
There was a problem hiding this comment.
The method LoadVersionDataSerializedSync loads the game version saved on disk into string format. However, there's no error handling for when the file does not exist or cannot be read. Consider adding appropriate error handling to ensure that the application can recover gracefully from such scenarios.
| /// Thrown if the given <paramref name="instance"/> doesn't implement all type interface | ||
| /// </exception> | ||
| /// <returns> | ||
| /// This installer reference to allow chain calls if necessayr |
There was a problem hiding this comment.
There's a typo in the comment. It should be "necessary" instead of "necessayr".
- /// This installer reference to allow chain calls if necessayr
+ /// This installer reference to allow chain calls if necessary|
|
||
| if (!type2.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface"); |
There was a problem hiding this comment.
The error message is incorrect, it should refer to type2 instead of type1.
- throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface");
+ throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface");|
|
||
| if (!type2.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface"); |
There was a problem hiding this comment.
The error message is incorrect, it should refer to type2 instead of type1.
- throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface");
+ throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface");|
|
||
| if (!type2.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface"); |
There was a problem hiding this comment.
The error message is incorrect, it should refer to type2 instead of type1.
- throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface");
+ throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface");| /// <inheritdoc /> | ||
| public IInstaller Bind<T1, T2, T3, T4>(T1 instance) where T1 : class, T2, T3, T4 | ||
| { | ||
| var type1 = typeof(T1); | ||
| var type2 = typeof(T2); | ||
| var type3 = typeof(T3); | ||
| var type4 = typeof(T4); | ||
|
|
||
| if (!type1.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface"); | ||
| } | ||
|
|
||
| if (!type2.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type2} is not an interface"); | ||
| } | ||
|
|
||
| if (!type3.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type3} is not an interface"); | ||
| } | ||
|
|
||
| if (!type4.IsInterface) | ||
| { | ||
| throw new ArgumentException($"Cannot bind {instance} because {type4} is not an interface"); | ||
| } | ||
|
|
||
| _bindings.Add(type1, instance); | ||
| _bindings.Add(type2, instance); | ||
| _bindings.Add(type3, instance); | ||
| _bindings.Add(type4, instance); | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
The code for checking whether the types are interfaces and adding them to the bindings is repeated multiple times. Consider creating a private method that performs these operations to adhere to the DRY principle.
+ private void BindType<T>(T instance) where T : class
+ {
+ var type = typeof(T);
+
+ if (!type.IsInterface)
+ {
+ throw new ArgumentException($"Cannot bind {instance} because {type} is not an interface");
+ }
+
+ _bindings.Add(type, instance);
+ }
- public IInstaller Bind<T1, T2>(T1 instance) where T1 : class, T2
- {
- var type1 = typeof(T1);
- var type2 = typeof(T2);
-
- if (!type1.IsInterface)
- {
- throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface");
- }
-
- if (!type2.IsInterface)
- {
- throw new ArgumentException($"Cannot bind {instance} because {type1} is not an interface");
- }
-
- _bindings.Add(type1, instance);
- _bindings.Add(type2, instance);
-
- return this;
- }
+ public IInstaller Bind<T1, T2>(T1 instance) where T1 : class, T2
+ {
+ BindType<T1>(instance);
+ BindType<T2>(instance);
+
+ return this;
+ }1 participant
Add the auto execute process to fill up the version service data based on the git information
Add the possibility to bind multiple interfaces in one call
Summary by CodeRabbit
GitEditorProcessclass for running Git commands from within Unity, allowing developers to perform version control operations directly in the editor.VersionEditorUtilsclass for managing the internal version of the game, providing a centralized way to handle versioning and updates.IInstallerinterface and its implementationInstallerto support binding multiple interfaces to a single instance, improving flexibility and modularity in dependency injection.Summary by CodeRabbit
GitEditorProcessclass to run Git commands as processes, enabling checks for valid Git repositories, retrieving current branch names, commit hashes, and diffs from given commits.VersionEditorUtilsclass for managing application versioning. This includes setting and saving the internal version before building, loading version data from disk, and generating an internal version suffix based on Git information and build settings.IInstallerinterface with new methods for binding multiple type interfaces to a single instance, improving modularity and code organization.