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

Allow extensible returning in handlers #66

Closed
The-EDev opened this issue Nov 21, 2020 · 7 comments · Fixed by #84
Closed

Allow extensible returning in handlers #66

The-EDev opened this issue Nov 21, 2020 · 7 comments · Fixed by #84
Assignees
Labels
feature Code based project improvement
Milestone

Comments

@The-EDev
Copy link
Member

The-EDev commented Nov 21, 2020

we have JSON mode, basically sets the content-type header to application/json, just make the same thing for multipart, instead of converting to string and returning that, something like:

    CROW_ROUTE(app,"/multipart")
    ([](const crow::request& req){
        crow::multipart::message msg(req);
        return msg;
    });
@The-EDev The-EDev added feature Code based project improvement good first issue an issue suitable to start contributing to the project labels Nov 21, 2020
@The-EDev The-EDev added this to the v0.3 milestone Nov 21, 2020
@The-EDev The-EDev changed the title Allow returning multipart Allow returning multipart in handlers Nov 21, 2020
@The-EDev
Copy link
Member Author

One solution for this issue + support for all other types would be to introduce a returnable class containing primarily a dump() and set_mode(crow::response res) methods, the dump() method would simply put the class' content into the response body, and the set_mode(crow::response res) would set the headers (specifically content-type) to the appropriate ones, then any user/developer can create a wrapper for any library they might want to return directly.

@The-EDev The-EDev changed the title Allow returning multipart in handlers Allow extensible returning in handlers Nov 27, 2020
@The-EDev The-EDev removed the good first issue an issue suitable to start contributing to the project label Nov 27, 2020
@The-EDev
Copy link
Member Author

so I was trying out yesterday's solution. it caused a circular dependency which I should've anticipated..

So I changed the method to just a content type variable, which for some reason didn't work. So I changed it to a method that returned the content type, which worked, here's the code.

struct returnable
{
    virtual const std::string dump() = 0;
    virtual const std::string content_type() = 0;
};

Implementation for content_type() in multipart::message:
const std::string content_type() override {return "multiplart/form-data";}

The child class would add an implementation to these, allowing it to be returned from a handler.
What I'm not sure of is if there's a better way to go about doing this.

@The-EDev The-EDev self-assigned this Nov 30, 2020
@The-EDev
Copy link
Member Author

The-EDev commented Dec 5, 2020

@mrozigor as I'm not as familiar with C++ as I would like to be, what do you think of this approach?, does it make sense? is there a better way to go about it?

@mrozigor
Copy link
Member

mrozigor commented Dec 7, 2020

This looks good - I think it is good to explicitly 'mark' class as returnable.

@NobodyXu
Copy link

NobodyXu commented Dec 28, 2020

@The-EDev IMHO it is better to use class for virtual base class and use struct for c-like aggregate.

It makes the code more readable and less confusing to new comers.

@The-EDev
Copy link
Member Author

Well I'm mainly following the existing code, as far as I understand, the main difference between struct and class is that one is public by default and the other is private

@NobodyXu
Copy link

But to new contributers, this code at the first glance look like a C-like structure (aggregate), not a virtual base class since there isn't a constructor or destructor.

Personally I think it is better to use class so that the reader immediately knows that this isn't a C-like structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Code based project improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants