-
Notifications
You must be signed in to change notification settings - Fork 11
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 multipart content type #30
Conversation
This is required for vrchatapi/specification#408 to be merged |
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.
The sed statement changes nothing for me on the master branch.
Even grep "TODO: support file upload for" src/apis/files_api.rs
comes back with nothing.
Did you want this to be a Draft PR?
That's corrcect, it only changes stuff from the following pr:
|
Okay, but then the documentation of the specification is wrong.
Yet in this pr you interpret file as a filename to open and read |
Yes, that's how the generators create it, the generators create a PathBuf, which (as I understand it) store a Path to a file on disk |
Then the description should read |
Also using a blocking file read in an async context is not ideal either in this scenario (though I can totally understand that choice) |
The issue with this is that it depends on the generator :) The C# generator takes in a byte stream |
Honestly the choice was because I don't know almost anything about rust async, I didnt realise that the file reading was sync, I can see if I can turn it into async in a bit |
With the current dependencies |
Ah Well realistically the images won't be too big so do we think that's worth it? I feel like it might be fine like this |
Signed-off-by: C0D3 M4513R <[email protected]>
Oh also you use |
Yeah that's fair lol |
@jellejurre Can you look at these changes? |
d22df3b
to
fa7b449
Compare
Wait, now that I'm thinking about it again, couldn't we just patch the sdk and have the user supply s byte stream and mime type? Is that a better approach? |
Looks good to me, works and gives a proper error when handing a non-existent file |
Also possible, fine by me and probably more extendable in the long run |
Writing a regex replace in regex101.com for that is no problem, but I can't get the regex to actually work. |
honestly, throw the regex in chatgpt, it's great at regexes lol |
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 finally got the regex.
I couldn't use sed, because it isn't powerful enough.
Only thing left is to test this again and then to merge (if you have perms, feel free to merge yourself. If not, I'll merge it, when you said, that it works) |
this works great, LGTM |
No description provided.