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

Feature: Make macros public #1431

Closed
MRuecklCC opened this issue Sep 29, 2022 · 13 comments
Closed

Feature: Make macros public #1431

MRuecklCC opened this issue Sep 29, 2022 · 13 comments

Comments

@MRuecklCC
Copy link

Feature Request

Make marcos in toplevel marco.rs public to other crates.

Motivation

I was just playing around with the custom extractors and rejections, and following along how this is done for JSON bodies in axum. I want to achieve something similar for other content types, and thought I can simply define my own rejections and composite rejections with the same macros. Unfortunately they are declared as pub(crate) so I cant get them :-(

I think they would be useful for other users as well. I assume pulling them into the public API would require some more documentation, besides that the change would be trivial(?).

Alternatives

I understand, that having a larger public API comes with responsibilities on stability etc. So this should be a well agreed upon decision (in either direction).

@jplatte
Copy link
Member

jplatte commented Sep 29, 2022

I don't think that's going to happen. I can't find an existing issue about this, but I'm pretty sure David has previously rejected a similar request.

The one thing in this direction I think we could add are IntoResponseParts and IntoResponse derive macros.

@davidpdrsn
Copy link
Member

I don't think that's going to happen. I can't find an existing issue about this, but I'm pretty sure David has previously rejected a similar request.

Yeah I don't think those macros should be public. Writing good macro_rules! macros is really hard so honestly I'd just recommend you copy/paste them into your code as necessary. I also don't think they're that necessary if you're just defining a couple of rejection types. But if you're defining dozens, like axum does, then they are indeed helpful.

@davidpdrsn davidpdrsn closed this as not planned Won't fix, can't repro, duplicate, stale Sep 29, 2022
@MRuecklCC
Copy link
Author

Good :) Totally understand! Thanks for the quick response and keep up the awesome work! <3

@davidpdrsn
Copy link
Member

The one thing in this direction I think we could add are IntoResponseParts and IntoResponse derive macros.

I'm up for having something like that!

@MRuecklCC
Copy link
Author

MRuecklCC commented Sep 29, 2022

The one thing in this direction I think we could add are IntoResponseParts and IntoResponse derive macros.

How would that look like? Following the cookbook maybe like this?

#[derive(IntoResponse)]
#[status = UNSUPPORTED_MEDIA_TYPE]
#[body = "Expected request with `Content-Type: text/sql`"]
pub struct MyRejectionReason;

I would like that even more than the currently (not) available macros :-)

@MRuecklCC
Copy link
Author

And, yes, I am playing around with SQL in the body. 🤷

@davidpdrsn
Copy link
Member

How would that look like?

No idea. Haven't thought about it.

And, yes, I am playing around with SQL in the body.

😱

@jplatte
Copy link
Member

jplatte commented Sep 29, 2022

I was thinking about the composite rejection case:

#[derive(IntoResponse)]
pub enum JsonRejection {
    JsonDataError(JsonDataError),
    JsonSyntaxError(JsonSyntaxError),
    MissingJsonContentType(MissingJsonContentType),
    BytesRejection(BytesRejection),
}

// generates
impl IntoResponse for JsonRejection {
    fn into_response(self) -> axum::response::Response {
        match self {
            JsonDataError(inner) => IntoResponse::into_response(inner),
            JsonSyntaxError(inner) => IntoResponse::into_response(inner),
            MissingJsonContentType(inner) => IntoResponse::into_response(inner),
            BytesRejection(inner) => IntoResponse::into_response(inner),
        }
    }
}

Though the struct case could also be done via derive, with something like you showed. Not sure whether it's worth it if you need to write half the lines you'd write for the impl as attributes instead.

@davidpdrsn
Copy link
Member

Yeah that would be

#[derive(IntoResponse)]
#[status = UNSUPPORTED_MEDIA_TYPE]
#[body = "Expected request with `Content-Type: text/sql`"]
pub struct MyRejectionReason;

// vs

pub struct MyRejectionReason;

impl IntoResponse for MyRejectionReason {
    fn into_response(self) -> Response {
        (
            StatusCode::UNSUPPORTED_MEDIA_TYPE,
            "Expected request with `Content-Type: text/sql`",
        ).into_response()
    }
}

@MRuecklCC
Copy link
Author

IMHO declarative > imperative. I would 100% prefer the macro way :)
Is there interest in this? I may have a couple hours the next days to prepare a PR. But be warned, I am rather new to rust :P

@davidpdrsn
Copy link
Member

I think the enum use case the jplatte shared in #1431 (comment) is fine.

I don't think

#[derive(IntoResponse)]
#[status = UNSUPPORTED_MEDIA_TYPE]
#[body = "Expected request with `Content-Type: text/sql`"]
pub struct MyRejectionReason;

offers much over writing your own IntoResponse version.

@MRuecklCC
Copy link
Author

MRuecklCC commented Sep 29, 2022

According to rust-lang/rust#52393 it is also not possible to use constants as attribute values. I.e. it would have to be a string...

#[derive(IntoResponse)]
#[status = "UNSUPPORTED_MEDIA_TYPE"] // not very nice, the name lookup would be in the derive macro implementation
// #[status = 415] // this would also work
#[body = "Expected request with `Content-Type: text/sql`"]
pub struct MyRejectionReason;

@jplatte
Copy link
Member

jplatte commented Sep 29, 2022

It is not possible for the proc-macro to read the value of the constant, but syntax-wise that should be no problem.

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

No branches or pull requests

3 participants