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 for SqlCachedBuffer.cs #1280

Merged
merged 10 commits into from
Oct 8, 2021

Conversation

lcheunglci
Copy link
Contributor

Relating to issue #1261 . I moved the SqlCacheBuffer.cs from netcore to shared src and updated the reference in the netfx csproj. I did notice that in the ToXmlReader method was implemented differently between the two. I looked at the helper method SqlTypeWorkarounds.SqlXmlCreateSqlXmlReader(...) and it seems to be doing the same functionality without using reflection.

@lcheunglci lcheunglci changed the title Move to shared SqlCachedBuffer Move into shared for SqlCachedBuffer.cs Sep 23, 2021
@lcheunglci
Copy link
Contributor Author

I decided to remove the ToXmlReader method because there's no references to it anywhere in the code and tests. After further comparison between the reflection implementation in netfx and the helper method in netcore, they both seems different but since it's an internal method and it's not being used, I feel like it's safe to remove.

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 23, 2021

Seems reasonable. We'll see if it affects any of the tests (which sometimes use private reflection) and if nothing is broken then it'll be a good cleanup.

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

Wraith2 commented Sep 29, 2021

Unless there are merge conflicts I wouldn't suggest keeping all your PR's up to date with main. It will take you a lot of time and it can be months before the team are ready to review.

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.

Modifiers could be reordered on ToString(): public override string ToString()

Comment on lines 130 to 132
public bool IsNull
{
get
{
return (_cachedBytes == null) ? true : false;
}
get => _cachedBytes == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: It would be simpler using expression body syntax:
public bool IsNull => _cachedBytes is null;

Comment on lines 134 to 135


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra lines; could be removed.

Comment on lines 33 to 36
internal List<byte[]> CachedBytes
{
get { return _cachedBytes; }
get => _cachedBytes;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: This way it would be neater.
internal List<byte[]> CachedBytes => _cachedBytes;

lcheunglci and others added 2 commits October 5, 2021 13:41
Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
@cheenamalhotra cheenamalhotra merged commit 9a7d53e into dotnet:main Oct 8, 2021
@lcheunglci lcheunglci deleted the MergeShared-SqlCachedBuffer 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.

6 participants