Add versioning support in durabletask-dotnet(phase1) #295
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,127 +8,169 @@ | |
| /// </summary> | ||
| public readonly struct TaskName : IEquatable<TaskName> | ||
| { | ||
| // TODO: Add detailed remarks that describe the role of TaskName | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="TaskName"/> struct. | ||
| /// </summary> | ||
| /// <param name="name">The name of the task. Providing <c>null</c> will yield the default struct.</param> | ||
| public TaskName(string name) | ||
| { | ||
| if (name is null) | ||
| { | ||
| // Force the default struct when null is passed in. | ||
| this.Name = null!; | ||
| this.Version = null!; | ||
| } | ||
| else | ||
| { | ||
| this.Name = name; | ||
| this.Version = string.Empty; // expose setting Version only when we actually consume it. | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="TaskName"/> struct. | ||
| /// </summary> | ||
| /// <param name="name">The name of the task. Providing <c>null</c> will yield the default struct.</param> | ||
| /// <param name="version">The version of the task.</param> | ||
cgillum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public TaskName(string name, string version) | ||
| { | ||
| if (name is null) | ||
|
There was a problem hiding this comment. This is a good opportunity for pattern matching. Can replace this if/else statements with: (this.Name, this.Version) = (name, version) switch
{
(null, null) => (null!, null!), // Force the default struct when null is passed in.
(null, string _) => throw new ArgumentException("name must not be null when version is non-null"),
(string _, null) => (name, string.Empty),
(string _, string _) => (name, version),
};Also while we are at it, can replace the other ctor with: (this.Name, this.Version) = name switch
{
null => (null!, null!), // Force the default struct when null is passed in.
string _ => (name, string.Empty),
} |
||
| { | ||
| if (version is null) | ||
| { | ||
| // Force the default struct when null is passed in. | ||
| this.Name = null!; | ||
| this.Version = null!; | ||
| } | ||
| else | ||
| { | ||
| throw new ArgumentException("name must not be null when version is non-null"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (version is null) | ||
| { | ||
| this.Name = name; | ||
| this.Version = string.Empty; // fallback to the contructor without version parameter | ||
skyao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| else | ||
| { | ||
| this.Name = name; | ||
| this.Version = version; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the name of the task without the version. | ||
| /// </summary> | ||
| /// <value> | ||
| /// The name of the activity task without the version. | ||
| /// </value> | ||
| public string Name { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the version of the task. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Task versions is currently locked to <see cref="string.Empty" /> as it is not yet integrated into task | ||
| /// identification. This is being left here as we intend to support it soon. | ||
skyao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// </remarks> | ||
| public string Version { get; } | ||
|
|
||
| /// <summary> | ||
| /// Implicitly converts a <see cref="TaskName"/> into a <see cref="string"/> of the <see cref="Name"/> property value. | ||
| /// </summary> | ||
| /// <param name="value">The <see cref="TaskName"/> to be converted into a string.</param> | ||
| public static implicit operator string(TaskName value) => value.ToString(); | ||
|
|
||
| /// <summary> | ||
| /// Implicitly converts a <see cref="string"/> into a <see cref="TaskName"/> value. | ||
| /// </summary> | ||
| /// <param name="value">The string to convert into a <see cref="TaskName"/>.</param> | ||
| public static implicit operator TaskName(string value) => string.IsNullOrEmpty(value) ? default : new(value); | ||
|
|
||
| /// <summary> | ||
| /// Compares two <see cref="TaskName"/> objects for equality. | ||
| /// </summary> | ||
| /// <param name="a">The first <see cref="TaskName"/> to compare.</param> | ||
| /// <param name="b">The second <see cref="TaskName"/> to compare.</param> | ||
| /// <returns><c>true</c> if the two <see cref="TaskName"/> objects are equal; otherwise <c>false</c>.</returns> | ||
| public static bool operator ==(TaskName a, TaskName b) | ||
| { | ||
| return a.Equals(b); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Compares two <see cref="TaskName"/> objects for inequality. | ||
| /// </summary> | ||
| /// <param name="a">The first <see cref="TaskName"/> to compare.</param> | ||
| /// <param name="b">The second <see cref="TaskName"/> to compare.</param> | ||
| /// <returns><c>true</c> if the two <see cref="TaskName"/> objects are not equal; otherwise <c>false</c>.</returns> | ||
| public static bool operator !=(TaskName a, TaskName b) | ||
| { | ||
| return !a.Equals(b); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether to <see cref="TaskName"/> objects | ||
| /// are equal using value semantics. | ||
| /// </summary> | ||
| /// <param name="other">The other object to compare to.</param> | ||
| /// <returns><c>true</c> if the two objects are equal using value semantics; otherwise <c>false</c>.</returns> | ||
| public bool Equals(TaskName other) | ||
| { | ||
| return string.Equals(this.Name, other.Name, StringComparison.OrdinalIgnoreCase) | ||
| && string.Equals(this.Version, other.Version, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether to <see cref="TaskName"/> objects | ||
| /// are equal using value semantics. | ||
| /// </summary> | ||
| /// <param name="obj">The other object to compare to.</param> | ||
| /// <returns><c>true</c> if the two objects are equal using value semantics; otherwise <c>false</c>.</returns> | ||
| public override bool Equals(object? obj) | ||
| { | ||
| if (obj is not TaskName other) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return this.Equals(other); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Calculates a hash code value for the current <see cref="TaskName"/> instance. | ||
| /// </summary> | ||
| /// <returns>A 32-bit hash code value.</returns> | ||
| public override int GetHashCode() | ||
| { | ||
| if (string.IsNullOrEmpty(this.Version)) | ||
| { | ||
| return StringComparer.OrdinalIgnoreCase.GetHashCode(this.Name); | ||
| } | ||
|
|
||
| return StringComparer.OrdinalIgnoreCase.GetHashCode(this.Name) * 31 | ||
|
Check warning on line 157 in src/Abstractions/TaskName.cs
|
||
skyao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| + StringComparer.OrdinalIgnoreCase.GetHashCode(this.Version); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the string value of the current <see cref="TaskName"/> instance. | ||
| /// </summary> | ||
| /// <returns>The name and optional version of the current <see cref="TaskName"/> instance.</returns> | ||
| public override string ToString() | ||
| { | ||
| if (string.IsNullOrEmpty(this.Version)) | ||
| { | ||
| return this.Name; | ||
| } | ||
| else | ||
| { | ||
| return this.Name + ":" + this.Version; | ||
|
There was a problem hiding this comment. We should consider adding FromString semantics. But I wonder if that will be a breaking change? We have not restricted what characters can be used in a task name before this. This change is going to introduce a confusing behavior: TaskName name1 = new("MyTask", "2"); // Name = "MyTask", Version = "2'
TaskName name2 = name1.ToString(); // implicit operator convers. Name = "MyTask:2", Version = ""There was a problem hiding this comment. Should the implicit operator be updated to parse out the version? I'm not super concerned about the breaking change since 99+% of .NET users should be relying on the function name, which already can't have special characters. However, we can take a look at Kusto data to confirm. There was a problem hiding this comment. Yes the implicit operator would be changed to call There was a problem hiding this comment. We should have a discussion on what separator to use. I like However, we already use There was a problem hiding this comment. I think using There was a problem hiding this comment. @cgillum yeah I am trying to think through how that behaves. One thing is that it is a separate type for entities. We have a few options:
There was a problem hiding this comment. Want to clarify that I am not opposed to a different separator char for version. We haven't officially GA'd distributed tracing v2, so we could update that spec with the new separator. But what one exactly? Some that I have ruled out myself in the past:
There was a problem hiding this comment. Can we merge this PR first and then open a new issue to trace and update the separator after we have a final decision? Because today is my last day, I'm afraid that I can't continue to update the code in this PR because of permissions. |
||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.