Tagging #if statements for Az #91
Conversation
…urceManager #if statements.
# Conflicts: # src/Authentication/Authentication/AdalTokenProvider.cs # src/Authentication/Authentication/ITokenProvider.cs # src/Authentication/Factories/AuthenticationFactory.cs
| userId, | ||
| password, | ||
| credentialType); | ||
| return _userTokenProvider.GetAccessToken(config, promptBehavior, promptAction, userId, password, credentialType); |
There was a problem hiding this comment.
why the two different styles of call? Above you move from a single line, here you move to a single line. Seems like we could just leave this alone altogether, or always use the multi-line call
There was a problem hiding this comment.
I don't remember changing this. It might have happened automatically when I renamed the field with the underscore. This is why I like the idea of having a code style that is applied to the entire codebase automatically. 😉
There was a problem hiding this comment.
As a ganeral rule, this is good, but this can run afoul of other engineering best practices (e.g. it is a good idea to make changes to legacy codebases minimal, to prevent introducing regressions)
| { | ||
| if (_store.FileExists(cacheFileName)) | ||
| if (!_store.FileExists(cacheFileName)) return; |
There was a problem hiding this comment.
Always enclose the code block of an if statement in brackets, even if there is only one statement in the block. This future-proofs you in case additional staatements are added to the block in revisions.
There was a problem hiding this comment.
Actually, that is the purpose of not using brackets. The code's intent is to not add additional code to the if-statement. It is a guard clause. In these instances, I don't even write that code. It is generated by Resharper, and is stylistically correct for the intent. However, I can understand this being confusing if you are not used to C#.
I can't find any specific materials on it, but I've always seen guard clauses as single line statements. However, if we want all if-statements to have brackets, we should enforce it via a style manager.
There was a problem hiding this comment.
First of all, none of these modificatiosn are necessary for the purpose of this PR, and they are holding up the PR as we have this discussion.
Secondly, there is a reason why no if statement block should ever lack brackets, and it has to do with engineering best practices for maintenance. If you don't have brackets, it is an easy mistake to place another statement under the if clause withoutadding the brackets ina future revision.
Additionally, this entire pattern adds an additional exit point for the function. It is not necessary for this change, and we should remove it.
| { | ||
| if (HasStateChanged) |
There was a problem hiding this comment.
Please revert this to the simple if
| { | ||
| IAzureTokenCache result = new AuthenticationStoreTokenCache(new AzureTokenCache()); | ||
| if (autoSaveMode == ContextSaveMode.CurrentUser) | ||
| if (autoSaveMode != ContextSaveMode.CurrentUser) return result; |
There was a problem hiding this comment.
See comments from Azure/azure-powershell#7712
There was a problem hiding this comment.
There are no comments there on this. See above, pleaser rever6t this - it is unrelated to the change that is the subject of this PR.
| @@ -34,7 +34,7 @@ public AuthenticationFactory() | |||
| { | |||
| _getKeyStore = () => | |||
| { | |||
| IServicePrincipalKeyStore keyStore = null; | |||
There was a problem hiding this comment.
I believe that some versions of CodeAnalysis will show this as uninitialized - let's leave in the explicit initialization
There was a problem hiding this comment.
This is not uninitialized. See comments from Azure/azure-powershell#7712
There was a problem hiding this comment.
I am aware of how the language works, I am talking about earlier versions of CodeAnalysis.
| @@ -325,7 +311,7 @@ public ServiceClientCredentials GetServiceClientCredentials(IAzureContext contex | |||
| tokenCache = context.TokenCache; | |||
| } | |||
|
|
|||
| ServiceClientCredentials result = null; | |||
There was a problem hiding this comment.
This is not uninitialized. See comments from Azure/azure-powershell#7712
| { | ||
| // make best effort to remove credentials | ||
| } | ||
| var cache = tokenCache as TokenCache; |
There was a problem hiding this comment.
See comments in Azure/azure-powershell#7712
There was a problem hiding this comment.
See comments above, please revert
| { | ||
| if (cache != null && cache.Count > 0 && account != null && !string.IsNullOrWhiteSpace(account.Id) && !string.IsNullOrWhiteSpace(account.Type)) | ||
| if (cache == null || cache.Count <= 0 || account == null || string.IsNullOrWhiteSpace(account.Id) || |
There was a problem hiding this comment.
See comments in Azure/azure-powershell#7712
There was a problem hiding this comment.
see comments above, please revert
| { | ||
| bool result = false; | ||
| if (account != null && !string.IsNullOrWhiteSpace(account.Type) && item != null) | ||
| if (account == null || string.IsNullOrWhiteSpace(account.Type) || item == null) return false; |
There was a problem hiding this comment.
See comments in Azure/azure-powershell#7712
There was a problem hiding this comment.
see comments above, please revert
| { | ||
| msg = exceptionMessage; | ||
| } | ||
| if (FileUtilities.DataStore.DirectoryExists(directory)) return; |
There was a problem hiding this comment.
See comments in Azure/azure-powershell#7712
There was a problem hiding this comment.
see comments above, please revert
|
For this row:
I don't see an ifdef in this file, what is the high risk change? |
|
@markcowl There used to be I'm assuming this was removed as part of Cormac's changes in this repo. |
|
@MiYanni do we still need this PR? |
|
Nope. |
This is for: Azure/azure-powershell#6793
Key
#if NETSTANDARDwas simply tagged with either// TODO: Remove IfDefor// TODO: Remove IfDef code. These changes are unlikely to cause issues and/or won't require any special attention for migration.#if NETSTANDARDwas removed and not necessary.