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

Merged SqlFilestream #2398

Closed
wants to merge 6 commits into from

Conversation

edwardneal
Copy link
Contributor

Relates to issue #1261.

This merges SqlFileStream between .NET Core and Framework. In the process, I've also implicitly dropped a code path to set the process-level or thread-level exception mode. This code path would set the thread-level exception mode on Windows 7 or higher, and the process-level exception mode on Windows XP or below; I've discarded the latter path.

SqlFileStream depends on a handful of PInvokes and a PathInternal class. I've merged these too, trimming unused methods where applicable. The removal of the former instances of these accounts for 70-80% of the removals.

I've also added ReadAsync and WriteAsync methods to SqlFileStream in .NET Core. The underlying FileStream supports these, so it doesn't do any harm to pass them through.

* Small variable name changes to fit style of the wider library.
* Made the EaNameString constant a compiler-level constant
* Added extra validation to support drop-in .NET Framework replacement

.NET Core compiles. Planning to use this as the baseline for the shared file.
…dAsync/WriteAsync methods.

Also updated the refs project, although I think this'll be regenerated as part of the build.
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

Attention: Patch coverage is 36.00000% with 80 lines in your changes missing coverage. Please review.

Project coverage is 72.64%. Comparing base (769b982) to head (8921b47).
Report is 157 commits behind head on main.

Files with missing lines Patch % Lines
...c/Microsoft/Data/SqlTypes/SqlFileStream.Windows.cs 44.55% 56 Missing ⚠️
...crosoft/Data/SqlTypes/SqlFileStream.Unsupported.cs 0.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2398      +/-   ##
==========================================
- Coverage   72.71%   72.64%   -0.07%     
==========================================
  Files         310      308       -2     
  Lines       61885    61535     -350     
==========================================
- Hits        44997    44703     -294     
+ Misses      16888    16832      -56     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 76.78% <31.62%> (-0.14%) ⬇️
netfx 70.14% <49.45%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benrr101
Copy link
Contributor

benrr101 commented Oct 7, 2024

Hey @edwardneal to be honest I started on merging this class before seeing your PR (I guess our list of class merges with open PRs wasn't completely up to date). Since it looks like there's a lot of conflicts in this PR now, would it be alright if we close this one and look at the one(s) I put together? I'd also really appreciate your feedback on them, since I see you made some changes to this class that I may have overlooked.

@edwardneal
Copy link
Contributor Author

Yes - no problem at all. I was hoping that the combination of our PInvoke moves/merges would tidy up a lot of the diff here, and wasn't looking forward to the merge which resulted from that. Your PRs already have that, so it makes sense to proceed with one of those.

@edwardneal edwardneal closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants