Skip to content

Attributes - #59

Merged
carllerche merged 2 commits into
carllerche:masterfrom
dtolnay:attrs
Aug 13, 2018
Merged

Attributes#59
carllerche merged 2 commits into
carllerche:masterfrom
dtolnay:attrs

Conversation

@dtolnay

@dtolnay dtolnay commented Aug 10, 2018

Copy link
Copy Markdown
Contributor

Fixes #56.

impl_web! {
    impl JsonResource {
        #[get("/")]
        fn hello_world(&self) -> Result<serde_json::Value, ()> {
            Ok(json!({ "message": "hello world" }))
        }
    }
}

@carllerche

Copy link
Copy Markdown
Owner

Nice... looking through the PR, the trade off would be that #![recursion_limit = "N"] would be required by users?

@dtolnay

dtolnay commented Aug 10, 2018

Copy link
Copy Markdown
Contributor Author

Yes -- this approach scans the tokens inside the impl block one at a time looking for tower-web attributes. There are ways to process multiple tokens per level of macro recursion to reduce the recursion depth. I pushed a commit to illustrate.

@carllerche

Copy link
Copy Markdown
Owner

cc @shepmaster

@shepmaster

Copy link
Copy Markdown
Collaborator

I always find myself (irrationally) annoyed at having to add #![recursion_limit="128"], but it is nice that when the "real" support is added that attribute can just be removed.

Converting my code to the attributes seemed straight-forward and things seemingly continue to work. Overall, I'm happy with the end result as an end-user.

@carllerche carllerche left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@carllerche
carllerche merged commit 2e1a1ef into carllerche:master Aug 13, 2018
@dtolnay
dtolnay deleted the attrs branch August 13, 2018 22:25
kornholi pushed a commit to kornholi/tower-web that referenced this pull request Dec 3, 2019
This adds the ability to support attributes on stable Rust while maintaining support
for good error messages.
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

Successfully merging this pull request may close these issues.

3 participants