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

Implement codegen's type lookup in a more extensible fashion #88

Closed
sgrif opened this issue Jan 12, 2016 · 2 comments
Closed

Implement codegen's type lookup in a more extensible fashion #88

sgrif opened this issue Jan 12, 2016 · 2 comments
Milestone

Comments

@sgrif
Copy link
Member

sgrif commented Jan 12, 2016

Currently in codegen, when we're attempting to figure out the NativeSqlType to map to for a table, we do the following:

  • Is the first character of the type name an _? If so ::diesel::types::Array<{next step}>
  • ::diesel::types::{type name with capitalized first letter}

This was a wart that was the "absolute minimum that could possibly work" to get 0.1 out the door, but it's time for it to go. (And it'll be nice to get rid of ugly little hacks like https://github.com/sgrif/diesel/blob/3ab23f6d20b4405a8361726fd9212e332b599061/diesel/src/types/mod.rs#L47).

The main constraint here is that the system we replace this with is something that third party crates can hook into. I also do not want to require that the type be in scope, so we need to get the full path (::diesel::types::* is rarely in scope, and I don't want it to be).

In an ideal world, third party crates could just do something like #[diesel_type_lookup(oid = "1234")] pub struct PostGisOrSomething, but I don't think that's possible (especially with the added constraint that I want to potentially take an arbitrary function), as I don't believe that we can have mutable state as part of a compilation unit that crosses crate boundaries.

So I think the best way we can actually do this is to have a database table. This means that other crates will have to actually populate it, but we can have a macro required or a function call required from build.rs. In the simplest case this can be a table that maps oid -> path to type, and we continue to special case arrays (though based on pg_type.typcategory instead of the name). If we don't want to special case arrays, I think we end up with a metadata entry that is an array of SQL function names. Each of these functions would take a pg_type row, and return the path to the modifier and another oid if it matches, or null if it doesn't.

In either case, all of Diesel's lookups should go through this same machinery. It should not be special cased.

@sgrif
Copy link
Member Author

sgrif commented Jan 23, 2016

Moving this to 1.0 as while it's important for launch, it's not immediately a problem

@sgrif
Copy link
Member Author

sgrif commented Jun 5, 2017

The basic idea of how the lookup works hasn't changed, but we do provide the ability to have that result in types from places other than diesel::types. I think this is sufficient until I see a use case showing otherwise.

@sgrif sgrif closed this as completed Jun 5, 2017
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

1 participant