Skip to content

Commit c947a74

Browse files
m-nashm-nash
authored
Pr feedback on the API (Azure#49775)
1 parent b803383 commit c947a74

29 files changed

+108
-80
lines changed

‎sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -262,11 +262,11 @@ public abstract partial class ModelReaderWriterTypeBuilder
262262
protected ModelReaderWriterTypeBuilder() { }
263263
protected abstract System.Type BuilderType { get; }
264264
protected virtual System.Type? ItemType { get { throw null; } }
265-
protected virtual void AddItem(object collection, object? item) { }
266-
protected virtual void AddKeyValuePair(object collection, string key, object? item) { }
265+
protected virtual void AddItem(object collectionBuilder, object? item) { }
266+
protected virtual void AddItemWithKey(object collectionBuilder, string key, object? item) { }
267+
protected virtual object ConvertCollectionBuilder(object collectionBuilder) { throw null; }
267268
protected abstract object CreateInstance();
268-
protected virtual System.Collections.IEnumerable? GetItems(object obj) { throw null; }
269-
protected virtual object ToCollection(object builder) { throw null; }
269+
protected virtual System.Collections.IEnumerable? GetItems(object collection) { throw null; }
270270
}
271271
public abstract partial class OperationResult
272272
{

‎sdk/core/System.ClientModel/api/System.ClientModel.net8.0.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -262,11 +262,11 @@ public abstract partial class ModelReaderWriterTypeBuilder
262262
protected ModelReaderWriterTypeBuilder() { }
263263
protected abstract System.Type BuilderType { get; }
264264
protected virtual System.Type? ItemType { get { throw null; } }
265-
protected virtual void AddItem(object collection, object? item) { }
266-
protected virtual void AddKeyValuePair(object collection, string key, object? item) { }
265+
protected virtual void AddItem(object collectionBuilder, object? item) { }
266+
protected virtual void AddItemWithKey(object collectionBuilder, string key, object? item) { }
267+
protected virtual object ConvertCollectionBuilder(object collectionBuilder) { throw null; }
267268
protected abstract object CreateInstance();
268-
protected virtual System.Collections.IEnumerable? GetItems(object obj) { throw null; }
269-
protected virtual object ToCollection(object builder) { throw null; }
269+
protected virtual System.Collections.IEnumerable? GetItems(object collection) { throw null; }
270270
}
271271
public abstract partial class OperationResult
272272
{

‎sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -262,11 +262,11 @@ public abstract partial class ModelReaderWriterTypeBuilder
262262
protected ModelReaderWriterTypeBuilder() { }
263263
protected abstract System.Type BuilderType { get; }
264264
protected virtual System.Type? ItemType { get { throw null; } }
265-
protected virtual void AddItem(object collection, object? item) { }
266-
protected virtual void AddKeyValuePair(object collection, string key, object? item) { }
265+
protected virtual void AddItem(object collectionBuilder, object? item) { }
266+
protected virtual void AddItemWithKey(object collectionBuilder, string key, object? item) { }
267+
protected virtual object ConvertCollectionBuilder(object collectionBuilder) { throw null; }
267268
protected abstract object CreateInstance();
268-
protected virtual System.Collections.IEnumerable? GetItems(object obj) { throw null; }
269-
protected virtual object ToCollection(object builder) { throw null; }
269+
protected virtual System.Collections.IEnumerable? GetItems(object collection) { throw null; }
270270
}
271271
public abstract partial class OperationResult
272272
{

‎sdk/core/System.ClientModel/gen/ModelReaderWriterContextGenerator.Emitter.cs‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections;
55
using System.Collections.Concurrent;
66
using System.Collections.Generic;
7+
using System.Runtime.InteropServices;
78
using System.Text;
89
using Microsoft.CodeAnalysis.Text;
910

@@ -283,6 +284,7 @@ private static HashSet<string> GetNamespaces(TypeBuilderSpec modelInfo)
283284
case TypeBuilderKind.ReadOnlyMemory:
284285
namespaces.Add("System.Collections");
285286
namespaces.Add("System.Collections.Generic");
287+
namespaces.Add("System.Runtime.InteropServices");
286288
break;
287289
case TypeBuilderKind.Array:
288290
namespaces.Add("System.Collections.Generic");
@@ -329,19 +331,19 @@ private static void EmitReadOnlyMemoryBuilder(
329331
builder.AppendLine($"<{elementType.FullyQualifiedName}>();");
330332
builder.AppendLine();
331333

332-
builder.AppendLine(indent, "protected override void AddItem(object collection, object item)");
334+
builder.AppendLine(indent, "protected override void AddItem(object collectionBuilder, object item)");
333335
indent++;
334336
builder.Append(indent, "=> ((");
335337
builder.AppendType(typeof(List<>));
336-
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collection).Add(({elementType.FullyQualifiedName})item);");
338+
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collectionBuilder).Add(({elementType.FullyQualifiedName})item);");
337339
indent--;
338340
builder.AppendLine();
339341

340-
builder.AppendLine(indent, "protected override object ToCollection(object builder)");
342+
builder.AppendLine(indent, "protected override object ConvertCollectionBuilder(object collectionBuilder)");
341343
indent++;
342344
builder.Append(indent, $"=> new {modelInfo.Type.FullyQualifiedName}(((");
343345
builder.AppendType(typeof(List<>));
344-
builder.AppendLine($"<{elementType.FullyQualifiedName}>)builder).ToArray());");
346+
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collectionBuilder).ToArray());");
345347
indent--;
346348
builder.AppendLine();
347349

@@ -353,15 +355,13 @@ private static void EmitReadOnlyMemoryBuilder(
353355
builder.AppendLine(indent, $"if (obj is {modelInfo.Type.FullyQualifiedName} rom)");
354356
builder.AppendLine(indent, "{");
355357
indent++;
356-
builder.AppendLine(indent, "for (int i = 0; i < rom.Length; i++)");
357-
builder.AppendLine(indent, "{");
358-
indent++;
359-
builder.AppendLine(indent, "yield return rom.Span[i];");
358+
builder.Append(indent, "return ");
359+
builder.AppendType(typeof(MemoryMarshal));
360+
builder.AppendLine($".ToEnumerable(rom);");
360361
indent--;
361362
builder.AppendLine(indent, "}");
362-
indent--;
363-
builder.AppendLine(indent, "}");
364-
builder.AppendLine(indent, "yield break;");
363+
builder.AppendLine();
364+
builder.AppendLine(indent, $"return null;");
365365
indent--;
366366
builder.AppendLine(indent, "}");
367367
});
@@ -395,22 +395,22 @@ private static void EmitMultiDimensionalArrayBuilder(
395395
builder.AppendLine("();");
396396
builder.AppendLine();
397397

398-
builder.AppendLine(indent, "protected override void AddItem(object collection, object item)");
398+
builder.AppendLine(indent, "protected override void AddItem(object collectionBuilder, object item)");
399399
indent++;
400400
builder.Append(indent, "=> ((");
401401
builder.AppendVariableList(modelInfo.Type.ArrayRank, elementType.FullyQualifiedName);
402-
builder.Append(")collection).Add((");
402+
builder.Append(")collectionBuilder).Add((");
403403
builder.AppendVariableList(modelInfo.Type.ArrayRank - 1, elementType.FullyQualifiedName);
404404
builder.AppendLine(")item);");
405405
indent--;
406406
builder.AppendLine();
407407

408-
builder.AppendLine(indent, "protected override object ToCollection(object builder)");
408+
builder.AppendLine(indent, "protected override object ConvertCollectionBuilder(object collectionBuilder)");
409409
builder.AppendLine(indent, "{");
410410
indent++;
411411
builder.Append(indent, "var instance = (");
412412
builder.AppendVariableList(modelInfo.Type.ArrayRank, elementType.FullyQualifiedName);
413-
builder.AppendLine(")builder;");
413+
builder.AppendLine(")collectionBuilder;");
414414
builder.AppendLine(indent, "int rowCount = instance.Count;");
415415
builder.AppendLine(indent, "int colCount = instance[0].Count;");
416416
builder.AppendLine(indent, $"{modelInfo.Type.Name} multiArray = new {elementType.FullyQualifiedName}[rowCount, colCount];");
@@ -457,19 +457,19 @@ private static void EmitArrayBuilder(
457457
builder.AppendLine($"<{elementType.FullyQualifiedName}>();");
458458
builder.AppendLine();
459459

460-
builder.AppendLine(indent, "protected override void AddItem(object collection, object item)");
460+
builder.AppendLine(indent, "protected override void AddItem(object collectionBuilder, object item)");
461461
indent++;
462462
builder.Append(indent, "=> ((");
463463
builder.AppendType(typeof(List<>));
464-
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collection).Add(({elementType.FullyQualifiedName})item);");
464+
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collectionBuilder).Add(({elementType.FullyQualifiedName})item);");
465465
indent--;
466466
builder.AppendLine();
467467

468-
builder.AppendLine(indent, "protected override object ToCollection(object builder)");
468+
builder.AppendLine(indent, "protected override object ConvertCollectionBuilder(object collectionBuilder)");
469469
indent++;
470470
builder.Append(indent, "=> ((");
471471
builder.AppendType(typeof(List<>));
472-
builder.AppendLine($"<{elementType.FullyQualifiedName}>)builder).ToArray();");
472+
builder.AppendLine($"<{elementType.FullyQualifiedName}>)collectionBuilder).ToArray();");
473473
indent--;
474474
});
475475
}
@@ -496,9 +496,9 @@ private static void EmitDictionaryBuilder(
496496
builder.AppendLine(indent, $"protected override object CreateInstance() => new {modelInfo.Type.FullyQualifiedName}();");
497497
builder.AppendLine();
498498

499-
builder.AppendLine(indent, "protected override void AddKeyValuePair(object collection, string key, object item)");
499+
builder.AppendLine(indent, "protected override void AddItemWithKey(object collectionBuilder, string key, object item)");
500500
indent++;
501-
builder.AppendLine(indent, $"=> (({modelInfo.Type.FullyQualifiedName})collection).Add(key, ({elementType.FullyQualifiedName})item);");
501+
builder.AppendLine(indent, $"=> (({modelInfo.Type.FullyQualifiedName})collectionBuilder).Add(key, ({elementType.FullyQualifiedName})item);");
502502
indent--;
503503
});
504504
}
@@ -525,9 +525,9 @@ private static void EmitListBuilder(
525525
builder.AppendLine(indent, $"protected override object CreateInstance() => new {modelInfo.Type.FullyQualifiedName}();");
526526
builder.AppendLine();
527527

528-
builder.AppendLine(indent, "protected override void AddItem(object collection, object item)");
528+
builder.AppendLine(indent, "protected override void AddItem(object collectionBuilder, object item)");
529529
indent++;
530-
builder.AppendLine(indent, $"=> (({modelInfo.Type.FullyQualifiedName})collection).Add(({elementType.FullyQualifiedName})item);");
530+
builder.AppendLine(indent, $"=> (({modelInfo.Type.FullyQualifiedName})collectionBuilder).Add(({elementType.FullyQualifiedName})item);");
531531
indent--;
532532
});
533533
}

‎sdk/core/System.ClientModel/src/ModelReaderWriter/ModelReaderWriterContext.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@ public ModelReaderWriterTypeBuilder GetTypeBuilder(Type type)
3333
/// <returns>True if the corresponding <see cref="ModelReaderWriterTypeBuilder"/> if defined in the context otherwise false.</returns>
3434
public bool TryGetTypeBuilder(Type type, [NotNullWhen(true)] out ModelReaderWriterTypeBuilder? builder)
3535
{
36-
if (TryGetTypeBuilderCore(type, out builder))
36+
if (TryGetTypeBuilderCore(type, out builder) && builder is not null)
3737
{
38-
builder!.Context = this;
38+
builder.Context = this;
3939
return true;
4040
}
4141

‎sdk/core/System.ClientModel/src/ModelReaderWriter/ModelReaderWriterTypeBuilder.CollectionWrapper.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public CollectionWrapper(ModelReaderWriterTypeBuilder builder, ModelReaderWriter
2121

2222
public object Builder => _instance ??= _builder.CreateInstance();
2323

24-
public object ToCollection() => _builder.ToCollection(AssertType(Builder, _builder.BuilderType));
24+
public object ToCollection() => _builder.ConvertCollectionBuilder(AssertType(Builder, _builder.BuilderType));
2525

2626
public void AddItem(object? item, string? key)
2727
{
@@ -32,7 +32,7 @@ public void AddItem(object? item, string? key)
3232

3333
if (key is not null)
3434
{
35-
_builder.AddKeyValuePair(AssertType(Builder, _builder.BuilderType), key, item);
35+
_builder.AddItemWithKey(AssertType(Builder, _builder.BuilderType), key, item);
3636
}
3737
else
3838
{

‎sdk/core/System.ClientModel/src/ModelReaderWriter/ModelReaderWriterTypeBuilder.cs‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,15 @@ namespace System.ClientModel.Primitives;
99
/// <summary>
1010
/// Provides an interface to create objects without needing reflection.
1111
/// </summary>
12+
/// <remarks>
13+
/// In most cases the implementation will be created automatically by the source generator. In some advanced scenarios
14+
/// the implementation may be created manually by the user see https://aka.ms/no-modelreaderwritertypebuilder-found for more details.
15+
///
16+
/// This class has no state and therefore the same instance can be used in multiple calls to <see cref="ModelReaderWriter"/>.
17+
/// <see cref="ModelReaderWriterContext"/> will cache the <see cref="ModelReaderWriterTypeBuilder"/> instances for each type.
18+
/// The state for collection building is maintained internally by <see cref="ModelReaderWriter"/> and is not needed to be maintained by the implementation.
19+
/// The instance of the collection builder will be passed into each method that needs to modify that state such as <see cref="AddItem"/> and <see cref="AddItemWithKey"/>.
20+
/// </remarks>
1221
public abstract partial class ModelReaderWriterTypeBuilder
1322
{
1423
/// <summary>
@@ -38,6 +47,10 @@ internal object CreateObject()
3847
/// <summary>
3948
/// Gets the type this builder creates.
4049
/// </summary>
50+
/// <remarks>
51+
/// For collections like an array which you cannot dynamically add to
52+
/// this should be the type of collection that will be used to build the array such as <see cref="List{T}"/>.
53+
/// </remarks>
4154
protected abstract Type BuilderType { get; }
4255

4356
/// <summary>
@@ -46,37 +59,54 @@ internal object CreateObject()
4659
protected virtual Type? ItemType => null;
4760

4861
/// <summary>
49-
/// Gets an <see cref="IEnumerable"/> representation of the object.
62+
/// Gets an <see cref="IEnumerable"/> representation of the passed in collection.
5063
/// </summary>
64+
/// <remarks>
65+
/// This is only needed when the collection passed into <see cref="ModelReaderWriter.Write(object, ModelReaderWriterOptions, ModelReaderWriterContext)"/>
66+
/// does not implement <see cref="IEnumerable"/>. In this case you must implement this method to return an <see cref="IEnumerable"/>.
67+
/// </remarks>
5168
/// <returns>An <see cref="IEnumerable"/> representation if its a collection otherwise null.</returns>
52-
protected virtual IEnumerable? GetItems(object obj) => null;
69+
protected virtual IEnumerable? GetItems(object collection) => null;
5370

5471
/// <summary>
5572
/// Creates and returns a new instance of the object type that this builder represents.
5673
/// </summary>
5774
protected abstract object CreateInstance();
5875

5976
/// <summary>
60-
/// Converts the input builder collection into the requested collection format.
77+
/// Converts the passed in builder collection into the requested collection type.
6178
/// </summary>
62-
/// <param name="builder">The builder collection that is being transformed.</param>
79+
/// <remarks>
80+
/// In the case like an array which you cannot dynamically add to <see cref="CreateInstance"/> would have returned an instance of
81+
/// <see cref="List{T}"/> which matches the <see cref="BuilderType"/>. This method would then convert the <see cref="List{T}"/>
82+
/// into an array by calling <see cref="List{T}.ToArray"/>.
83+
/// </remarks>
84+
/// <param name="collectionBuilder">The builder collection that is being transformed.</param>
6385
/// <returns>The requested collection format.</returns>
64-
protected virtual object ToCollection(object builder) => builder;
86+
protected virtual object ConvertCollectionBuilder(object collectionBuilder) => collectionBuilder;
6587

6688
/// <summary>
67-
/// Adds an item to a specified collection.
89+
/// Adds an item to the passed in collection builder.
6890
/// </summary>
69-
/// <param name="collection">Represents the collection to which the item will be added.</param>
70-
/// <param name="item">Represents the item that will be added to the collection.</param>
71-
protected virtual void AddItem(object collection, object? item)
91+
/// <remarks>
92+
/// The collection builder instance that is being passed in is the instance returned from <see cref="CreateInstance"/>.
93+
/// The state of the collection builder is maintained internally by <see cref="ModelReaderWriter"/> and is not needed to be maintained by the implementation.
94+
/// </remarks>
95+
/// <param name="collectionBuilder">Represents the collection builder to which the item will be added.</param>
96+
/// <param name="item">Represents the item that will be added to the collection builder.</param>
97+
protected virtual void AddItem(object collectionBuilder, object? item)
7298
=> Debug.Fail("AddItem should not be called for non-collections types.");
7399

74100
/// <summary>
75-
/// Adds an item to a specified dictionary under the specified key.
101+
/// Adds an item to the passed in collection builder under the specified key.
76102
/// </summary>
77-
/// <param name="collection">Represents the collection to which the item will be added.</param>
103+
/// <remarks>
104+
/// The collection builder instance that is being passed in is the instance returned from <see cref="CreateInstance"/>.
105+
/// The state of the collection builder is maintained internally by <see cref="ModelReaderWriter"/> and is not needed to be maintained by the implementation.
106+
/// </remarks>
107+
/// <param name="collectionBuilder">Represents the collection builder to which the item will be added.</param>
78108
/// <param name="key">Represents the key under which the item will be added.</param>
79-
/// <param name="item">Represents the item that will be added to the collection.</param>
80-
protected virtual void AddKeyValuePair(object collection, string key, object? item)
81-
=> Debug.Fail("AddKeyValuePair should not be called for non-dictionary collections.");
109+
/// <param name="item">Represents the item that will be added to the collection builder.</param>
110+
protected virtual void AddItemWithKey(object collectionBuilder, string key, object? item)
111+
=> Debug.Fail("AddItemWithKey should not be called for non-dictionary collections.");
82112
}

‎sdk/core/System.ClientModel/src/ModelReaderWriter/ReflectionContext.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
using System.Collections.Generic;
5-
64
namespace System.ClientModel.Primitives;
75

86
internal class ReflectionContext : ModelReaderWriterContext

0 commit comments

Comments
 (0)