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

Add write file stream #290

Merged
merged 3 commits into from
Sep 5, 2023
Merged

Conversation

garcipat
Copy link
Contributor

@garcipat garcipat commented Sep 4, 2023

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.

@CLAassistant
Copy link

CLAassistant commented Sep 4, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ejsmith ejsmith left a 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) {
Copy link
Contributor

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.

Copy link
Contributor

@ejsmith ejsmith left a comment

Choose a reason for hiding this comment

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

LGTM

@ejsmith ejsmith merged commit 871be38 into FoundatioFx:master Sep 5, 2023
1 check passed
@ejsmith
Copy link
Contributor

ejsmith commented Sep 5, 2023

Thank you @garcipat! Will you be able to create PRs for the other storage implementations as well?

@garcipat
Copy link
Contributor Author

garcipat commented Sep 5, 2023

What you mean? I went though all implementations of IFileStorage and added that. Or did I miss something?
Edit: ah okay those are in other repositories. Will check that tomorrow.

@ejsmith
Copy link
Contributor

ejsmith commented Sep 5, 2023

Yeah, not sure which ones you are using, but there are several IFileStorage implementations for S3, Azure Blob Storage, SshNet, Minio, Aliyun, and Redis. If you are using any of those and want to do PRs for those, that would be awesome. :-) If you are feeling really generous and want to go do PRs for all of them that would be crazy cool. :-)

@garcipat
Copy link
Contributor Author

garcipat commented Sep 5, 2023

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.

@niemyjski
Copy link
Member

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?

@garcipat
Copy link
Contributor Author

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.
I think it would be preferable to set it intentionally when changing from the obsolete method since its not obvious that the stream is only reading if nothing set. You only discover that when trying to write and it doesnt work.

But thats personal preference I guess.

@ejsmith
Copy link
Contributor

ejsmith commented Sep 22, 2023

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.

garcipat added a commit to garcipat/Foundatio that referenced this pull request Sep 23, 2023
@garcipat garcipat deleted the add-write-file-stream branch September 23, 2023 10:31
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.

None yet

4 participants