Send file by api v3 #78
Conversation
GustavoCaso
force-pushed
the
serve_images_in_documentation
branch
from
839fdd8 to
a2556d6
Compare
test/v3/routes/tracks_test.rb
Outdated
There was a problem hiding this comment.
It would recommend also verifying that Content-Type header is set to image/png and not application/json.
There was a problem hiding this comment.
Definitely @groyoh. Thanks for pointing out
There was a problem hiding this comment.
We should verify that we're serving the correct type on jpgs, I think. It would be easy to hard-code 'image/png' as the type.
There was a problem hiding this comment.
Actually, that test could probably be at the unit test level.
There was a problem hiding this comment.
What does camera have to do with this? How about just test.png?
GustavoCaso
force-pushed
the
serve_images_in_documentation
branch
from
a2556d6 to
54d1a90
Compare
|
Yeah, I don't want to do the base64 encoding, since we're just serving an image as a file, not as part of a JSON response. |
v3/routes/tracks.rb
Outdated
There was a problem hiding this comment.
It might be cleaner if this were an object:
img = config.find(id).docs.image(filename)
img.exists?
img.path
img.type
|
Great @kytrinyx .I will finish with this one today and keep working on the other ones. |
GustavoCaso
force-pushed
the
serve_images_in_documentation
branch
2 times, most recently
from
2af3095 to
7da293e
Compare
|
@kytrinyx Are we removing the old readme endpoint ?? |
GustavoCaso
force-pushed
the
serve_images_in_documentation
branch
from
7da293e to
448fdbc
Compare
|
@kytrinyx Is there anything else you want me to do for this feature ? |
|
I'm not sure which endpoint you mean. We'll eventually get rid of the entire api v1-v2, but the readme endpoint in v3 looks good now, and is ready to be used in the readme page on the site. |
lib/xapi/track/image.rb
Outdated
There was a problem hiding this comment.
Not a big deal here, but usually if you're inside of a class named Image there's no need to prefix anything with image on the inside, so I would probably have named this just name.
|
Sorry I don't know what I was thinking I think I was really tire 😞 |
|
Oh, having read your code I now understand your question. No, we're not getting rid of the README endpoint, but I rearranged a couple things yesterday while planning how to refactor some things in the website codebase. Look in |
|
No worries :) Things move really quickly sometimes, and there are so many moving pieces it's hard to keep track. |
|
Thanks for the patience I will clear the Pull Request, of the unwanted files |
|
Thank you! |
|
One thing before this is ready--would you make sure that we get a 404 rather than a 500 if the track doesn't exist or the docs directory doesn't exist? |
|
Sure no problem
|
GustavoCaso
force-pushed
the
serve_images_in_documentation
branch
from
448fdbc to
6bf54f8
Compare
|
@kytrinyx Hope this time I got it right, and once agin thanks for the patience |
There was a problem hiding this comment.
The common idiom here would be assert image.exists? rather than using assert_equal.
|
There are a couple of tiny style things, but they can be fixed in a later PR. Thanks for taking the time to work on this. |
|
Thanks @kytrinyx I haven't use MiniTest much |
3 participants
Issue: #53
@kytrinyx I follow your feedback from the issue, and implemented.
I have some concerns about encode base64 the image, usually it grows the size by 33%(Internet talking), we could Gzipped.
I don't know what you think about it.
@groyoh