-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
Added Renderable#data method to be able to respond with IO and String from an Action #1220
Added Renderable#data method to be able to respond with IO and String from an Action #1220
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.
I think overall this looks great. I know several people have asked for something similar, so this will be a huge addition. I'm not familiar with sending data to the client like this, so the only thing I'm wondering is do we need to be concerned with sending too large of data, or any sort of weird security bugs like requesting with headers that should be disallowed? 🤷♂️
Other than that, just one small comment and it should be good to go 👍
) : Lucky::DataResponse | ||
Lucky::DataResponse.new(context, io.gets_to_end, content_type, disposition, filename, status) | ||
end | ||
|
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.
Maybe we should also add an overload that takes HTTP::Status
like the others? Just to keep those consistent.
@igor-alexandrov If you want to throw a couple code snippets of using this functionality into this PR, I'm happy to take those over to the website and help document the new feature if you haven't done so already. |
@stephendolan the most obvious example looks like: class Reports::MyReport < ApiAction
get "/reports/my_report" do
result = CSV.build do |csv|
csv.row "one", "two"
csv.row "three"
end
data result, filename: "my_report.csv"
end
end Now I have doubts, do we really need |
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.
This looks great. I think just String
probably makes sense since you can stringify an IO with to_s
anyway.
Let me know when you've made the change and we can get this merged. Thanks so much!
@paulcsmith done, I also added specs for |
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!
This PR closes #1219.