-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
Add write file stream #290
Conversation
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.
Looks great! Thank you! Only bit of feedback is just to deprecate the old method so we can get rid of it since it is redundant now.
public Task<Stream> GetFileStreamAsync(string path, CancellationToken cancellationToken = default) | ||
=> GetFileStreamAsync(path, FileAccess.Read, cancellationToken); | ||
|
||
public Task<Stream> GetFileStreamAsync(string path, FileAccess fileAccess, CancellationToken cancellationToken = default) { |
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.
I like this signature. I would say that we should mark the current method as deprecated so that we can get rid of it on the next major version.
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.
LGTM
Thank you @garcipat! Will you be able to create PRs for the other storage implementations as well? |
What you mean? I went though all implementations of IFileStorage and added that. Or did I miss something? |
Yeah, not sure which ones you are using, but there are several |
Yes I rememeber. I means whats why we use fhe package. But I didnt realize its in other repositories. will see what I can do. Depends on how complicate it is with the other things. Also I dont know if I can test it with all of them. Will see. I will at least intend it. |
We probably want FileAccess fileAccess to default to Read, otherwise this is a pretty big breaking change. I upgraded a project and had quite a few compiler errors (warnings as errors on). The default for all of them was read. Thoughts? |
Currently the implementation without the file access does use read to prevemt a breaking change. But we could set a default value if you would like to if we change it again anyway. But thats personal preference I guess. |
It's not a breaking change because the old signature didn't have fileaccess on it and you would hit that one. The new method requires FileAccess and it should continue to do so even when we remove the old one. Also, @garcipat is going to update the enum to be a foundatio enum so that we control what stream modes are available. Which we said would just be read or write for now. |
Add write file stream (FoundatioFx#290)
To write files in the file storage, usually you want to write to the stream directly, not pass the stream over as an argument. (as I SaveFileAsync()
This PR adds an overload where you can tell if you want to use the stream you get to read or to write only.
There should be no breaking changes on existing things, but a new method is added, therefore implementations have to implement that.