Azure.Security.KeyVault.Certificates implementation #7530
Conversation
schaabs
commented
Sep 9, 2019
schaabs
commented
- APIs on CertificateClient implemented
- Model type serialization implemented
- Initial CertificateClient Live / Recorded Tests
|
|
||
| public static readonly Action EmailContacts = new Action(EmailContactsValue); | ||
|
|
||
| public override bool Equals(object obj) |
There was a problem hiding this comment.
May as well implement IEquitable<Action> which provides facilities to a lot of other uses (like hashing, comparison, etc.) without the boxing for object.Equals. Then just implement this as:
public override bool Equals(object obj) => obj is Action action && Equals(action);|
|
||
| public override int GetHashCode() | ||
| { | ||
| return base.GetHashCode(); |
There was a problem hiding this comment.
Given the _value here is really the crux of the enum-like structs, should we instead return _value?.GetHashCode() ?? 0 instead?
|
|
||
| public override string ToString() | ||
| { | ||
| return _value; |
There was a problem hiding this comment.
Could be null, and ToString() should never return null or empty: https://docs.microsoft.com/en-us/dotnet/api/system.object.tostring?view=netframework-4.8#notes-to-inheritors. Perhaps we should not allow either into the constructor to avoid this possibility.
| return _value; | ||
| } | ||
|
|
||
| public static bool operator ==(Action a, Action b) => a.Equals(b); |
There was a problem hiding this comment.
This would box the Action. See comment above about implementing IEquitable<Action> to avoid this.
|
|
||
| namespace Azure.Security.KeyVault.Certificates | ||
| { | ||
| public class AdministratorDetails |
There was a problem hiding this comment.
Should this be implementing IJsonSerializable and IJsonDeserializable?
| Properties = pollResponse; | ||
| } | ||
|
|
||
| if (Properties.Status == "completed") |
There was a problem hiding this comment.
Nit: we define "magic numbers" (err, strings) elsewhere throughout code. May as well here.
| } | ||
| private void ParseId(string id) | ||
| { | ||
| var idToParse = new Uri(id, UriKind.Absolute); ; |
There was a problem hiding this comment.
I, too, love var but seems our coding style is to use explicit type names. 😢
| private const string CancellationRequestedPropertyName = "cancellation_requested"; | ||
| private static readonly JsonEncodedText CancellationRequestedPropertyNameBytes = JsonEncodedText.Encode(CancellationRequestedPropertyName); | ||
|
|
||
| private bool _cancellationRequested; |
| Upn | ||
| } | ||
|
|
||
| //public enum CertificateContentType |
There was a problem hiding this comment.
Guessing this should just be deleted.
|
|
||
| namespace Azure.Security.KeyVault.Certificates.Tests | ||
| { | ||
| public class CertificatesTestBase : RecordedTestBase |
| internal const string AutoRenewValue = "AutoRenew"; | ||
| internal const string EmailContactsValue = "EmailContacts"; | ||
|
|
||
| public Action(string Action) |
There was a problem hiding this comment.
Should be lowercase "action". Also, for consistency, do we want all these values throughout structs like this to just be "value"?
|
/azp run net - keyvault - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| return obj is CertificateContentType && string.CompareOrdinal(_value, ((CertificateContentType)obj)._value) == 0; | ||
| } | ||
|
|
||
| public override int GetHashCode() |
There was a problem hiding this comment.
Why have an override that doesn't do anything?
There was a problem hiding this comment.
Then we should have a real implementation because we can do a better (faster) job than the default implementation.
| /// </summary> | ||
| public static readonly CertificateContentType Pem = new CertificateContentType("application/x-pem"); | ||
|
|
||
| public override bool Equals(object obj) |
There was a problem hiding this comment.
Would be nice to have a non-boxing equality operator
| /// <summary> | ||
| /// Supported JsonWebKey key types (kty) | ||
| /// </summary> | ||
| public struct CertificateKeyType |
* Azure.Security.KeyVault.Certificates initial implementation * adding test recordings * Fixing faux enums Equals and GetHashCode * squash incomplete xml comments * adding xml doc comments * updating keyvault release notes * removing duplicate refernce to shared source file
3 participants
