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 SqlConnectionPoolKey.cs #1292

Merged

Conversation

lcheunglci
Copy link
Contributor

Relates to #1261 . Merged the netfx version of SqlConnectionPoolKey.cs into netcore, and move it to the share src. I did notice that the netfx uses the ICloneable interface and the netcore version overrides the Clone method, and I went with the netcore version and verified it still compiled successfully on netfx.

@lcheunglci lcheunglci changed the title Move to Shared SqlConnectionPoolKey.cs Move to Shared for SqlConnectionPoolKey.cs Sep 27, 2021
@cheenamalhotra cheenamalhotra added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Sep 29, 2021
@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview3 milestone Sep 29, 2021
@DavoudEshtehari
Copy link
Contributor

My preference is to avoid #if #else directives as much as possible. Please, see this comment for more explanation.

@cheenamalhotra
Copy link
Member

I agree, please combine code together in a single if-def as much as possible, or in a separate partial class with *****.netfx.cs / *****.netcore.cs file, depending on the amount of per-framework code.

@lcheunglci
Copy link
Contributor Author

Thanks! I revised my changes based on @DavoudEshtehari comments from #1261 (comment) renaming the shared src with the .Common.cs prefix and made it a partial class so that the netcore and netfx version SqlConnectionPoolKey.cs can be back in its original location without using ifdefs

@lcheunglci
Copy link
Contributor Author

After a discussion with @DavoudEshtehari , we decided that it might be better to combine the netfx and netcore back into one file in shared src instead of introducing the partial class with their individual netfx and netcore implementations because the file is fairly small, so it's still relatively easy to read and maintain.

…hod CalculateHashCode and Equals and move the fields and properties to the top
Copy link
Contributor

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

Plz, remove the unused namespace here.

NIT: I'd suggest to follow the same pattern for all the similar changes in a file; My preference is to see the final if-else block at the end right before closing the class definition.

@lcheunglci
Copy link
Contributor Author

NIT: I'd suggest to follow the same pattern for all the similar changes in a file; My preference is to see the final if-else block at the end right before closing the class definition.

I did consider that but I found that having the fields, properties and constructors at the top easier to follow than to find it mixed at the end.

@DavoudEshtehari
Copy link
Contributor

Could you resolve the conflict, plz?

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 fix rebased check-in.

@cheenamalhotra cheenamalhotra merged commit 542056a into dotnet:main Oct 18, 2021
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