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 #1154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Apr 3, 2022

Fixes #1095.

I think this is technically breaking, because you can use an enum's variant to bring it into scope, but this is not allowed for constants. So code that does this would break:

use rusqlite::DbConfig::SQLITE_DBCONFIG_ENABLE_FKEY;
conn.db_config(SQLITE_DBCONFIG_ENABLE_FKEY)

Short of that, other cases should not be broken. EDIT: This was not possible for IndexConstraintOp, since it had the SQLITE_INDEX_CONSTRAINT_FUNCTION variant. I doubt many people use this, because we don't expose xFindFunction, so I just renamed it.

Other notes:

  1. Cases which don't correspond to SQLite's constants (for example, UNKNOWN variants) are present as associated constants, but have had the capitalized versions deprecated, since the "compatibility with SQLite C API" argument doesn't hold. Some of these probably also should be TryFrom impls rather than From.

  2. Some DbConfig values were gated on feature = "modern_sqlite". This has been removed, in favor of noting that they will fail if you try to use it in an unsupported version. This is useful if you want to toggle the feature if it's supported, but do nothing if not. For example, it seems reasonable to allow code like the following:

    if let Err(e) = conn.set_db_config(rusqlite::DbConfig::Defensive, true) {
        // Might be in an old version of SQLite, continue anyway...
        log::warn!("Failed to enable defensive mode: {:?}", e);
    }
  3. I only bothered with the value tests for cases where there were a large number of variants.

  4. In several cases, I also tried to improve the docs.

@thomcc thomcc requested a review from gwenn April 3, 2022 18:52
@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #1154 (d52300c) into master (9699b4a) will increase coverage by 0.22%.
The diff coverage is 82.25%.

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
+ Coverage   78.22%   78.45%   +0.22%     
==========================================
  Files          47       47              
  Lines        5929     5926       -3     
==========================================
+ Hits         4638     4649      +11     
+ Misses       1291     1277      -14     
Impacted Files Coverage Δ
src/session.rs 58.27% <16.66%> (+0.03%) ⬆️
src/hooks.rs 88.51% <25.00%> (ø)
src/config.rs 84.21% <66.66%> (-1.51%) ⬇️
src/vtab/mod.rs 55.39% <95.65%> (+4.20%) ⬆️
src/limits.rs 97.22% <95.83%> (-2.78%) ⬇️
src/vtab/array.rs 84.93% <100.00%> (ø)
src/vtab/series.rs 83.08% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9699b4a...d52300c. Read the comment docs.

Copy link
Collaborator

@gwenn gwenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have drop old entries.

@thomcc
Copy link
Member Author

thomcc commented Apr 4, 2022

You should have drop old entries.

Hm, you mean I should delete the old variants?

If you feel that way, I'll deprecate and #[doc(hidden)] them. After a couple versions we can delete them. I don't want people to dread updating rusqlite, and deleting them all in one go would break very many users.

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.

Rename enum variants to Rust-friendly names
2 participants