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

Adds rust types to reserved names. #1207

Closed
wants to merge 1 commit into from

Conversation

Emilgardis
Copy link

Fixes #1204

@Emilgardis
Copy link
Author

I considered sorting the (un)signed types properly, but I thought it wouldn't really matter.
Also, should we care about columns like "i32"?

@sgrif
Copy link
Member

sgrif commented Sep 26, 2017

I'd rather avoid speculatively doing this for all types, as there's no reason you couldn't just have a column called char. The only reason that bool is causing an issue is that it appears in the signature of a trait. I believe we can fix that by just changing impl_query_id! to use ::std::bool instead of bool, and not touch codegen at all

@sgrif
Copy link
Member

sgrif commented Sep 26, 2017

Hm... Actually ::std::bool doesn't work. I wonder if there even is a way to reference bool with an absolute path

@sgrif
Copy link
Member

sgrif commented Sep 26, 2017

I've opened rust-lang/rust#44865 to see if we can get a better way to do this. In the short term I think we should add this to the crate root:

// Referenced by `impl_query_id!` to ensure we get the right type if users have
// a column called `bool`. This should be removed when the language lets us
// reference `bool` with an absolute path
#[doc(hidden)]
pub type RealBool = bool;

and change impl_query_id! to use $crate::RealBool instead of bool. We should also add tests for this. Simply adding a migration with a column that has the same name as each primitive should suffice. Your list is also missing str

@sgrif
Copy link
Member

sgrif commented Jan 8, 2018

I'm going to close this, as it's gotten out of date. I would still like to see the underlying issue fixed, but through the method I mentioned above rather than making these names reserved.

@sgrif sgrif closed this Jan 8, 2018
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.

2 participants