[ACR] Rename ArtifactManifestProperties.Size->SizeInBytes

[Mohit-Chakraborty]

No description provided.

[Mohit-Chakraborty]

@annelo-msft, @KrzysztofCwalina - the one other potential rename we wanted to investigate was for RegistryArtifact.GetTagPropertiesCollection().

From the feedback I got, we can use the Collection suffix as we have now. Another option is to use All, giving us GetTagProperties() and GetAllTagProperties() as the two variants.

So, the choice is between GetTagPropertiesCollection() and GetAllTagProperties() for the pageable version of the method.
I like what we have currently, want to get feedback to update in this PR.

[azure-sdk]

API changes have been detected in Azure.Containers.ContainerRegistry. You can review API changes here

API changes

-         public long? Size { get; }
+         public long? SizeInBytes { get; }
-         public static ArtifactManifestProperties ArtifactManifestProperties(string registryLoginServer = null, string repositoryName = null, string digest = null, long? size = null, DateTimeOffset createdOn = default, DateTimeOffset lastUpdatedOn = default, ArtifactArchitecture? architecture = null, ArtifactOperatingSystem? operatingSystem = null, IEnumerable<ArtifactManifestPlatform> relatedArtifacts = null, IEnumerable<string> tags = null, bool? canDelete = null, bool? canWrite = null, bool? canList = null, bool? canRead = null);
+         public static ArtifactManifestProperties ArtifactManifestProperties(string registryLoginServer = null, string repositoryName = null, string digest = null, long? sizeInBytes = null, DateTimeOffset createdOn = default, DateTimeOffset lastUpdatedOn = default, ArtifactArchitecture? architecture = null, ArtifactOperatingSystem? operatingSystem = null, IEnumerable<ArtifactManifestPlatform> relatedArtifacts = null, IEnumerable<string> tags = null, bool? canDelete = null, bool? canWrite = null, bool? canList = null, bool? canRead = null);

[annelo-msft]

So, the choice is between GetTagPropertiesCollection() and GetAllTagProperties() for the pageable version of the method. I like what we have currently, want to get feedback to update in this PR.

Were you able to look into what others HLC libraries do when we have this plural situation? I believe @KrzysztofCwalina's guidance was to be consistent with the existing precedent if there is one. In LLC, we use the All to disambiguate.

[Mohit-Chakraborty]

So, the choice is between GetTagPropertiesCollection() and GetAllTagProperties() for the pageable version of the method. I like what we have currently, want to get feedback to update in this PR.

Were you able to look into what others HLC libraries do when we have this plural situation? I believe @KrzysztofCwalina's guidance was to be consistent with the existing precedent if there is one. In LLC, we use the All to disambiguate.

I didn't find any instance of this particular case. There are several places where we use the Collection suffix, not necessarily for this reason though. That is to clarify that the method returns a collection or an instance of a type that represents a collection.

[annelo-msft]

If we go with the Collection suffix over including All, we'll want to change LLC to adopt this convention as well. Let's get @KrzysztofCwalina's perspective on this.

[annelo-msft]

Per @KrzysztofCwalina:

I prefer the GetAll convention. The "Collections" suffix sounds like a Hungarian convention, i.e. refers to .NET type name as opposed to API semantics.

Also, I think the "All" convention is used in some .NET APIs, but not the "Collections" convention.

Having said that the "All" convention does not work in all cases. For example, imagine an API that returns a collection of items/resources but takes a filter as a parameter. It might be misleading to call the method GetAll(string filter) given it won't return all items unless the filter is "*".

It would be good to think what we would use if we ran into such APIs with a filter.

[azure-sdk]

API changes have been detected in Azure.Containers.ContainerRegistry. You can review API changes here

[Mohit-Chakraborty]

@annelo-msft, @KrzysztofCwalina - I have updated the API names as per our discussion.

I will add an entry to the guidelines to the effect of -
GetAll<resource_name> when <resource_name> is itself in plural form, like Properties.

[annelo-msft]

Cool, I see we're aligning with the LLC precedent. Thanks for doing this!