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

Use i64 for Model and Deck ids #11

Merged
merged 3 commits into from
Jan 14, 2023
Merged

Conversation

Heliozoa
Copy link
Contributor

@Heliozoa Heliozoa commented Dec 6, 2022

Hi,
Thanks for your work on genanki-rs.

I got an error when using a randomly generated Model id because it was too large (out of range integral type conversion attempted). The error occurred here:

genanki-rs/src/note.rs

Lines 168 to 185 in 6ddffe9

transaction
.execute(
"INSERT INTO notes VALUES(?,?,?,?,?,?,?,?,?,?,?);",
params![
id_gen.next(), // id
self.get_guid(), // guid
self.model.id, // mid
timestamp as i64, // mod
-1, // usn
self.format_tags(), // TODO tags
self.format_fields(), // flds
self.sort_field, // sfld
0, // csum, can be ignored
0, // flags
"", // data
],
)
.map_err(database_error)?;

I was also able to reproduce a similar error by trying to use a large Deck id. It seems like the cause is that the integer used in SQLite is an i64. Anki doesn't seem to mind negative model or deck ids, so this PR changes the ids to be i64 to avoid this kind of error. Note ids seem to be unaffected so I didn't change them.

Copy link

@lquenti lquenti left a comment

Choose a reason for hiding this comment

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

Looks great! Also fixes my problem. Please merge asap and update on crates.io

@yannickfunk
Copy link
Owner

LGTM, I will make a new crates.io release soon

@yannickfunk yannickfunk merged commit a4205f3 into yannickfunk:master Jan 14, 2023
@Heliozoa Heliozoa deleted the model-id branch January 14, 2023 19:40
@yannickfunk
Copy link
Owner

New version is out: https://crates.io/crates/genanki-rs/0.3.1

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.

3 participants