Fix FileUploadService #1288
Fix FileUploadService #1288
Conversation
ob-stripe
force-pushed
the
ob-fix-file-upload
branch
from
b8a9959 to
ee913ab
Compare
| @@ -18,34 +18,35 @@ public FileUploadService(string apiKey) | |||
| { | |||
| } | |||
|
|
|||
| public virtual FileUpload Create(string fileName, Stream fileStream, string purpose, RequestOptions requestOptions = null) | |||
| public virtual FileUpload Create(FileUploadCreateOptions options = null, RequestOptions requestOptions = null) | |||
There was a problem hiding this comment.
wouldn't options be required for file upload creation?
| { | ||
| fileName = "blob"; | ||
| fileName = fileStream.Name; |
There was a problem hiding this comment.
Isn't it easier to just put the filename in the options instead of trying to get it from the stream? And if it's not in the options we just use blob? I am not a fan of those pre-processor directives as we don't really test them thoroughly
There was a problem hiding this comment.
I think adding options that are not actual API parameters would be a bad precedent. Also, this method has the advantage of not requiring the user to manually provide the filename.
Unfortunately, even if this worked with .NET Standard, I don't think we could test without mocking :/
There was a problem hiding this comment.
Yep agreed I am just worried a little bit that we're making assumptions but only testing on OSX and all that without knowing how mono and other environments will fare. I might recommend asking @anelder-stripe about this.
I agree on the name that is not an API parameter but we use the filename in all our API examples too.
Why don't we just make filename an optional parameter by the way in the API itself?
There was a problem hiding this comment.
Re. testing, we do test with .NET Framework on AppVeyor, so we know that the code compiles and runs without errors at least (which in this instance is as much as we can say with .NET Core anyway).
Mono is supposed to be largely compatible with .NET Framework, but we've never tested against it (or offered any guarantee that the library works with Mono for that matter).
Why don't we just make filename an optional parameter by the way in the API itself?
The file is encoded as described in RFC 2388, which already includes a way of (optionally) providing the filename.
In that regard, I guess we could add a field for the filename in the options class, but even if we do I'd still be in favor of keeping the code I added to automatically provide the filename if it's not manually provided.
There was a problem hiding this comment.
Yeah I mean I'm fine with what you do, I'm just raising that .NET is still a platform we don't know as much about where environments would/could matter.
But I'm fine with merging as is, I just wanted to raise an alternative because I never understood why we didn't just make the filename a real argument instead.
| private FileUploadService service; | ||
| private FileUploadCreateOptions createOptions; |
There was a problem hiding this comment.
I don't know why the file above had permissions changed?
There was a problem hiding this comment.
I briefly modified the file when I tried to make the filename thing work with .NET Standard. I ended up reverting those changes but it looks like my editor fixed the permissions ¯\_(ツ)_/¯
ob-stripe
force-pushed
the
ob-fix-file-upload
branch
from
ee913ab to
eda665d
Compare
r? @remi-stripe
cc @stripe/api-libraries
Changes
FileUploadService.Createto accept a newFileUploadCreateOptionsclass instead of separate arguments.