-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
FbBlobStream #127
FbBlobStream #127
Conversation
Nice. Please give me some time to review it, as I'm currently busy. |
src/FirebirdSql.Data.FirebirdClient/FirebirdClient/FbBlobStream.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/FirebirdClient/FbBlobStream.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/FirebirdClient/FbBlobStream.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
Hey Jiri, i have addressed your feedback in this updated commit! Thanks a lot for having an in-depth look at the changes! |
@la-we Sorry for not taking another look sooner. I was busy with work and stuff. I'll do another pass in upcoming days. |
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.
In general looks good. Just some small nits.
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
src/FirebirdSql.Data.FirebirdClient/Client/Managed/Version10/GdsBlob.cs
Outdated
Show resolved
Hide resolved
|
||
if (buffer.Length == 0) | ||
{ | ||
// previous segment was last, this has no data |
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.
Fix spaces in comment.
@la-we could you rebase with the latest This would be a nice addition to the project. |
@cincuranet Addressed your feedback. Sorry for the long waiting period... |
@la-we Few pending comments left, mostly formatting. Otherwise LGTM and I'll merge after these are addressed. |
@la-we Are you aware of the failing test?
|
Nope, did not notice to be honest. I'll have a look at the failing tests |
@cincuranet Tests are fixed! |
Nice. I'll fix the #127 (comment) post merge. Thanks for your contribution! |
Added FbBlobStream to be able to stream a blob from the database rather than having to fetch it completely. Especially useful when dealing with large BLOBs.
This is a working but possibly not clean solution. Have a look whether something should be cleaned up or improved.