Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to Shared for SqlQueryMetadataCache.cs #1316

Merged

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261. Move the netcore version into shared src and updated the references in the csprojs. During the update to the conform to the coding style, I left a IDE1019, pattern matching, on line 64 on the info message list because I find the original way easier to follow than the suggestion.

@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Oct 5, 2021
@DavoudEshtehari DavoudEshtehari added this to the 4.0.0-preview3 milestone Oct 5, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Oct 5, 2021

The original:

Dictionary<string, SqlCipherMetadata> cipherMetadataDictionary = _cache.Get(cacheLookupKey) as Dictionary<string, SqlCipherMetadata>;

// If we had a cache miss just return false.
if (cipherMetadataDictionary is null)
{
    IncrementCacheMisses();
    return false;
}

the pattern matched alternative:

if (_cache.Get(cacheLookupKey) is not Dictionary<string, SqlCipherMetadata> cipherMetadataDictionary)
{
    IncrementCacheMisses();
    return false;
}

Since the cipherMetadataDictionary variable is used after the if statement that declares it I agree with you that it is clearer to use the original version.

@cheenamalhotra cheenamalhotra merged commit 2f0a801 into dotnet:main Oct 5, 2021
@lcheunglci lcheunglci deleted the MergeShared-SqlQueryMetadataCache branch October 14, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants