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

Where do we want to use associated consts? #1135

Closed
killercup opened this Issue Aug 31, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@killercup
Member

killercup commented Aug 31, 2017

Rust 1.20 stabilized associated consts!

…and, in contrast to every other hipster unstable Rust feature under the sun, we apparently have neither code comments nor commit messages in diesel's code base telling us what places could benefit from this new feature. But not to worry, we can easily[^1] find some!

Wait—should we? It makes the API a bit more idiomatic at the cost of requiring at least 1.12 to build diesel. Additionally, we could declare the current methods as #[deprecated] and not break any builds. The constants should be in SCREAMING_SNAKE_CASE anyway, so the names won't conflict. I'd say: Go for it, and actually replace these static methods. Right now, before 1.0, we can afford these breaking changes, after that we need to live with the status quo.

Here are the places[^1] that I found that could benefit from associated consts:

  • diesel::query_builder::QueryId: const HAS_STATIC_QUERY_ID: bool instead of fn has_static_query_id() ->bool
  • diesel::query_source::Column: const NAME: &'static str instead of fn name() -> &'static str
  • diesel::types::FromSqlRow: const FIELDS_NEEDED: usize (with default) instead of fn fields_needed() -> usize { ... }

I'm not entirely sure if we want/can/should turn methods like foo() -> Self::Foo into associated consts. This would add a few more cases (<Foo as diesel::associations::HasTable>::table(), <Foo as diesel::query_source::Table>::all_columns(), and <Foo as diesel::associations::BelongsTo>::foreign_key_column()).


[^1]: rg -i ' (pub | ) fn (.*?)\(\) ->'

@sgrif

This comment has been minimized.

Member

sgrif commented Sep 1, 2017

👍 for using associated constants for those 3 cases

@willmurphyscode

This comment has been minimized.

Contributor

willmurphyscode commented Sep 4, 2017

Is anyone presently working on this? If not, I'd like to volunteer to give this a try.

@Eijebong

This comment has been minimized.

Member

Eijebong commented Sep 4, 2017

I don't think anyone is working on it so feel free to work on it.
If you have any question don't hesitate to ask here or on our gitter room (link it the README)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment