Skip to content

Move into Shared SqlDependencyListener.cs #1308

New issue

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

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

Already on GitHub? # to your account

Merged

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261 . Merged netfx to netcore and move it to shared src. While cleaning up the coding style errorlist, I left 2 IDE0063 (Line 957 and Line 1076) info messages because I chose to keep the scope bodied using statement and there was a IDE0079 suggesting that I remove the suppression of CA2100:ReviewSqlQueriesForSecurityVulnerabilities on Line 968, which I left. I may need to merge PR #1299 because there's a small change to the property to the property to retrieve the static instance of SingletonProcessDispatcher.

@DavoudEshtehari DavoudEshtehari added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Oct 4, 2021
@DavoudEshtehari DavoudEshtehari added this to the 4.0.0-preview3 milestone Oct 4, 2021
Co-authored-by: Johnny Pham <v-jopha@microsoft.com>
@johnnypham
Copy link
Contributor

Some events were using SqlClientEventSource.Log.TryNotificationTraceEvent in netfx and SqlClientEventSource.Log.TryTraceEvent in netcore. Not sure which one we should use. Maybe someone else can comment.

@lcheunglci
Copy link
Contributor Author

Some events were using SqlClientEventSource.Log.TryNotificationTraceEvent in netfx and SqlClientEventSource.Log.TryTraceEvent in netcore. Not sure which one we should use. Maybe someone else can comment.

I noticed that too. I'm not sure if we need to unify those and if so which one should we be using.

@JRahnama
Copy link
Contributor

JRahnama commented Oct 8, 2021

@lcheunglci can you address the conflict please?

@lcheunglci
Copy link
Contributor Author

Some events were using SqlClientEventSource.Log.TryNotificationTraceEvent in netfx and SqlClientEventSource.Log.TryTraceEvent in netcore. Not sure which one we should use. Maybe someone else can comment.

I noticed that too. I'm not sure if we need to unify those and if so which one should we be using.

After a discussion with @cheenamalhotra and @JRahnama , I believe the consensus is to use the one from netfx. I'll make the updates in the next commit.

@Kaur-Parminder
Copy link
Contributor

NIT: @lcheunglci There are 'new' expression simplification info suggestions for line 87,1636,1713.

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reset branch to contain only PR related changes.

@lcheunglci lcheunglci force-pushed the MergeShared-SqlDependencyListener branch from 442930f to a21542f Compare October 16, 2021 03:06
@cheenamalhotra cheenamalhotra merged commit 85fbc16 into dotnet:main Oct 18, 2021
@lcheunglci lcheunglci deleted the MergeShared-SqlDependencyListener branch October 18, 2021 20:50
# for free to join this conversation on GitHub. Already have an account? # 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.

6 participants