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.

Sign up for GitHub

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

Jump to bottom

RFC: remove StateData trait #487

Open
lucacasonato opened this issue Oct 23, 2020 · 4 comments · May be fixed by #488
Open

RFC: remove StateData trait #487

lucacasonato opened this issue Oct 23, 2020 · 4 comments · May be fixed by #488
Labels
breaking-change discussion proposal

Comments

@lucacasonato
Copy link
Contributor

lucacasonato commented Oct 23, 2020 •
edited
Loading

Currently to be able to store something in State, the data needs to implement the StateData trait. This StateData trait is really just an alias for Any + Send. It would be really nice if implementing this trait would not be needed. Here are some reasons why:

  1. Not technically needed. This abstraction is not actually needed. As you can see in this branch, the StateData can be removed and replaced with direct Any + Send without issues. There is thus no technical reason for the existence of this trait.
  2. Unnecessary boilerplate. To be able to store a struct in gotham::state::State, you need to either add the StateData proc-macro to the declaration, or manually implement gotham::state::StateData. This is an extra line of code that is not needed.
  3. Forces unnecessary wrapper structs. If you want to store a structure from a crate outside of yours in State, you need to wrap it with of a wrapper struct just so you can implement StateData. This adds unnecessary bloat.

I propose that the StateData trait and StateData proc-macro are removed. All methods on State and FromState that currently take a StateData would take any Any + Send instead.

More benefits:

  • This would also be one less thing that users need to learn about when starting out with Gotham.
  • Removes ~150 LOC from gotham and examples.
@lucacasonato lucacasonato linked a pull request Oct 23, 2020 that will close this issue
Open
@msrd0
Copy link
Member

msrd0 commented Oct 23, 2020

I just looked through the history of gotham and it looks like StateData was introduced together with State, so it's not a leftover from a previous refactor. Does anyone know what the initial reason was to introduce that trait?

@msrd0
Copy link
Member

msrd0 commented Nov 3, 2020

Considering #494, removing StateData might not be something we'd want to do. If a middleware sometimes puts type Foo into your state and sometimes doesn't, we could map Option<Foo> to State::try_take<Foo>. This would be impossible if Option<Foo> also implemented StateData (or, equivalently, we removed the trait).

@lucacasonato
Copy link
Contributor Author

This would be impossible if Option also implemented StateData

How so? You could still make the opinionated assumption that Option is mapped to try_take. Possibly you could even force this in the type system (ie not let the user store Option in state).

Personally I think not being able to store non crate structures directly in the state is a far greater annoyance.

@msrd0
Copy link
Member

msrd0 commented Nov 3, 2020

How so? You could still make the opinionated assumption that Option is mapped to try_take.

Well, impl<T: Any> FromState for T using take and impl<T: Any> FromState for Option<T> using try_take would clash because Option<T> also implements Any (but does not implement StateData).

I'll wait to see an actual implementation, maybe there's a clever trick that I didn't think of that makes this possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking-change discussion proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: remove StateData trait lucacasonato/gotham
2 participants
@lucacasonato @msrd0