Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Editor.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

88 changes: 88 additions & 0 deletions Editor/GitEditorProcess.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
using System;
using System.Diagnostics;

namespace GameLovers.Services.Editor
Comment on lines +1 to +4
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

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
Comment on lines +6 to +12
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

Consider adding XML documentation comments to provide descriptions for the class and its members.

{
private const string DefaultPathToGitBinary = "git";

Comment on lines +14 to +15
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

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.

private readonly Process Process;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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;


/// <summary>
/// <inheritdoc cref="Process.ExitCode"/>
/// </summary>
public int ExitCode => Process.ExitCode;

public GitEditorProcess(string workingDir, string pathToGitBinary = DefaultPathToGitBinary)
{
var startInfo = new ProcessStartInfo
{
UseShellExecute = false,
RedirectStandardOutput = true,
FileName = pathToGitBinary,
CreateNoWindow = true,
WorkingDirectory = workingDir
};
Comment on lines +23 to +32
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

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.


Process = new Process { StartInfo = startInfo };
}
Comment on lines +23 to +35
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Is this unity project a git repo?
/// </summary>
public bool IsValidRepo()
{
return ExecuteCommand("rev-parse --is-inside-work-tree") == "true";
}

Comment on lines +37 to +44
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The IsValidRepo method could be simplified by directly returning the result of the ExecuteCommand method call.

/// <summary>
/// Get the git branch name of the unity project.
/// </summary>
public string GetBranch()
{
return ExecuteCommand("rev-parse --abbrev-ref HEAD");
}

/// <summary>
/// Get the git commit hash of the unity project.
/// </summary>
public string GetCommitHash()
{
return ExecuteCommand($"rev-parse HEAD");
}
Comment on lines +48 to +59
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The GetBranch and GetCommitHash methods could also be simplified by directly returning the result of the ExecuteCommand method calls.


/// <summary>
/// Get the diff of the working directory in its current state from the state it was at at
/// a given commit.
/// </summary>
public string GetDiffFromCommit(string commitHash)
{
return ExecuteCommand($"diff --word-diff=porcelain {commitHash} -- {Process.StartInfo.WorkingDirectory}");
}
Comment on lines +61 to +68
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The GetDiffFromCommit method could be improved by using the --relative option with the diff command to make the path argument relative to the repository root. This would allow the method to work correctly even if the working directory is not the repository root.


/// <inheritdoc/>
public void Dispose()
{
Process?.Dispose();
}
Comment on lines +71 to +74
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

Consider implementing the IDisposable interface explicitly to indicate that the Dispose method is intended to be called explicitly.


/// <summary>
/// Execute a command eg. "status --verbose"
/// </summary>
private string ExecuteCommand(string args)
{
Process.StartInfo.Arguments = args;
Process.Start();
var output = Process.StandardOutput.ReadToEnd().Trim();
Process.WaitForExit();
return output;
Comment on lines +79 to +85
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

Consider using the using statement to automatically dispose of the Process object instead of manually calling Dispose in the Dispose method.

Here's an updated version of the code with the suggested changes:

using System;
using System.Diagnostics;

namespace GameLovers.Utils
{
    /// <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
    {
        private readonly Process _process;

        /// <summary>
        /// <inheritdoc cref="Process.ExitCode"/>
        /// </summary>
        public int ExitCode => _process.ExitCode;

        public GitEditorProcess(string workingDir, string pathToGitBinary = "git")
        {
            var startInfo = new ProcessStartInfo
            {
                UseShellExecute = false,
                RedirectStandardOutput = true,
                FileName = pathToGitBinary,
                CreateNoWindow = true,
                WorkingDirectory = workingDir
            };

            _process = new Process { StartInfo = startInfo };
        }

        /// <summary>
        /// Is this unity project a git repo?
        /// </summary>
        public bool IsValidRepo()
        {
            return ExecuteCommand("rev-parse --is-inside-work-tree") == "true";
        }

        /// <summary>
        /// Get the git branch name of the unity project.
        /// </summary>
        public string GetBranch()
        {
            return ExecuteCommand("rev-parse --abbrev-ref HEAD");
        }

        /// <summary>
        /// Get the git commit hash of the unity project.
        /// </summary>
        public string GetCommitHash()
        {
            return ExecuteCommand("rev-parse HEAD");
        }

        /// <summary>
        /// Get the diff of the working directory in its current state from the state it was at at
        /// a given commit.
        /// </summary>
        public string GetDiffFromCommit(string commitHash)
        {
            return ExecuteCommand($"diff --word-diff=porcelain --relative {commitHash}");
        }

        /// <inheritdoc/>
        void IDisposable.Dispose()
        {
            _process?.Dispose();
        }

        /// <summary>
        /// Execute a command eg. "status --verbose"
        /// </summary>
        private string ExecuteCommand(string args)
        {
            _process.StartInfo.Arguments = args;
            _process.Start();
            var output = _process.StandardOutput.ReadToEnd().Trim();
            _process.WaitForExit();
            return output;
        }
    }
}

Please review the changes and let me know if you have any questions or concerns.

Comment on lines +81 to +85
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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}");
+ }

}
}
}
11 changes: 11 additions & 0 deletions Editor/GitEditorProcess.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

141 changes: 141 additions & 0 deletions Editor/VersionEditorUtils.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
using System;
using System.IO;
using UnityEditor;
using UnityEngine;

namespace GameLovers.Services.Editor
{
/// <summary>
/// Set the internal version in any VersionService instances in the project before building
/// and whenever the project loads.
/// </summary>
public static class VersionEditorUtils
{
private const int ShortenedCommitLength = 8;

Comment on lines +13 to +15
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The ShortenedCommitLength constant is not used in the code. It can be removed.

- private const int ShortenedCommitLength = 8;

/// <summary>
/// Set the internal version before building the app.
/// </summary>
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);
}
Comment on lines +19 to +31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The SetAndSaveInternalVersion method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:

  1. Extract the logic for generating the new version data into a separate method.
  2. Move the comparison of old and new version data to a separate method.
  3. 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);
}

Comment on lines +19 to +31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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;
- }


/// <summary>
/// Loads the game version saved in disk into string format
/// </summary>
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;
Comment on lines +36 to +47
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

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;
}

}
Comment on lines +36 to +48
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Set the internal version for when the app plays in editor.
/// </summary>
[InitializeOnLoadMethod]
private static void OnEditorLoad()
{
SetAndSaveInternalVersion(false);
}

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.");
}
Comment on lines +65 to +98
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the GenerateInternalVersionSuffix method, you're catching all exceptions but only logging them and issuing a warning. It might be better to rethrow the exception after logging it, so that the caller can decide how to handle it. This would also make debugging easier as the stack trace would be preserved.

- Debug.LogException(e);
- Debug.LogWarning("Could not execute git commands. Internal version not set.");
+ throw;

}
Comment on lines +59 to +99
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The GenerateInternalVersionSuffix method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:

  1. Extract the logic for retrieving git branch and commit into separate methods.
  2. 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)));
}
Comment on lines +101 to +139
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Image description CodeRabbit

The SaveVersionData method has a long and complex implementation. Consider refactoring it to improve readability and maintainability. Here are some suggestions:

  1. Use Path.Combine to concatenate file paths instead of manual string concatenation.
  2. Use File.WriteAllText to simplify writing the serialized data to a text file.
  3. Use AssetDatabase.ImportAsset to 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)));
}

Comment on lines +113 to +139
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The SaveVersionData method creates a directory if it doesn't exist, deletes an old file with an incorrect extension, and then writes the serialized data to a new text file. However, there's no error handling for any of these operations. Consider adding appropriate error handling to ensure that the application can recover gracefully from any errors that occur during these operations.

}
}
11 changes: 11 additions & 0 deletions Editor/VersionEditorUtils.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading