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

Rename enum variants to Rust-friendly names #1095

Open
thomcc opened this issue Jan 9, 2022 · 2 comments · May be fixed by #1154
Open

Rename enum variants to Rust-friendly names #1095

thomcc opened this issue Jan 9, 2022 · 2 comments · May be fixed by #1154

Comments

@thomcc
Copy link
Member

thomcc commented Jan 9, 2022

We have a lot of enumerations where the variants have names that are identical to the FFI names, which is bad style and looks weird.

However, the SQLite FFI names are still pretty important, and it would be a mistake for us not to clearly explain what each variant is. These are how the official docs will refer to these values, and it will help folks figure out what SQLite docs to read to understand the value, and what variant the official docs are referring to when they reference some variant.

That is, for a given enumeration, users should to easily be able to both: determine the Rust name for a given FFI name, and vice versa. (It also would be an enormous breaking change if we change all of this at once without some consideration for easy migration path)

To solve these issues, I propose that we do the following steps:

  1. Rename the variants to use CamelCase names.
  2. Reference the FFI names in the documentation of the variants.
  3. Add inherent constants with the SQLITE_ to both allow people to find them, and significantly reduce the breakage this change would cause.

I don't mind doing this, but @gwenn, do you have strong objections to this?

Example

For example, on Limit steps 1 and 2 would look like:

// ... attributes/docs omitted ...
pub enum Limit {
    /// The maximum size of any string or BLOB or table row, in bytes.
    ///
    /// Equivalent to `SQLITE_LIMIT_LENGTH` from the C API.
    Length = ffi::SQLITE_LIMIT_LENGTH,
    /// The maximum length of an SQL statement, in bytes.
    ///
    /// Equivalent to `SQLITE_LIMIT_SQL_LENGTH` from the C API.
    SqlLength = ffi::SQLITE_LIMIT_SQL_LENGTH,
    /// The maximum number of columns in a table definition or in the result set
    /// of a SELECT or the maximum number of columns in an index or in an
    /// ORDER BY or GROUP BY clause.
    ///
    /// Equivalent to `SQLITE_LIMIT_COLUMN` from the C API.
    Columns = ffi::SQLITE_LIMIT_COLUMN,
	// ... rest of the limits ...
}

And then step 3 would be providing the following:

impl Limit {
    /// An alias for [`Limit::Length`], provided for compatibility with the C API.
    pub const SQLITE_LIMIT_LENGTH: Self = Self::Length;
    /// An alias for [`Limit::SqlLength`], provided for compatibility with the C API.
    pub const SQLITE_LIMIT_SQL_LENGTH: Self = Self::SqlLength;
    /// An alias for [`Limit::Columns`], provided for compatibility with the C API.
    pub const SQLITE_LIMIT_COLUMN: Self = Self::Columns;
	// ... rest of the limits ...
}
@gwenn
Copy link
Collaborator

gwenn commented Jan 9, 2022

I am bad at (re)naming things but ok.

@thomcc
Copy link
Member Author

thomcc commented Jan 9, 2022

That's fine, I don't mind doing it. I just wanted to know if you were okay with it.

@thomcc thomcc self-assigned this Jan 9, 2022
@thomcc thomcc linked a pull request Apr 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants