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 into Shared SqlDependencyListener.cs #1308

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
@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
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.

6 participants