-
Notifications
You must be signed in to change notification settings - Fork 292
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
Share SqlBuffer #1438
Share SqlBuffer #1438
Conversation
The failures don't appear related to my changes, can someone re-run the failed legs? |
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
can you address the conflicts here please? |
dea707b
to
5cfd6dd
Compare
Done. |
_isNull = false; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of doing this could not we have #if directive
inside one class? this creates a new file again for netfx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but it would be inconsistent with the approach I've previously been asked to use. The approach has been to use conditional elements within the project file and avoid #ifdef blocks in individual files favouring totally separate files where possible.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
Outdated
Show resolved
Hide resolved
3b46c82
to
8b35520
Compare
Rebased and fixed. Please don't make me to this a third time. |
This ports the changes from dotnet/corefx#31044 to netfx. It then shares the resulting SqlBuffer file and uses a SqlBuffer.netfx.cs file to contain the netfx (legacy) methods required for v200 smi support.
@DavoudEshtehari this adds to the codebase merging