search

kerrickstaley / genanki

A Python 3 library for generating Anki decks
MIT License
1.99k stars 150 forks source link

Fix Card Order and Date of Creation Bug With Indexes #35

Closed georgjaehnig closed 3 years ago

georgjaehnig commented 5 years ago

I've implemented the solution suggested here: https://github.com/kerrickstaley/genanki/pull/30#discussion_r293843839

kerrickstaley commented 5 years ago

Hi @georgjaehnig, can you figure out why the Travis CI tests are failing? I can't merge this PR until they pass. Let me know if you need help.

kerrickstaley commented 5 years ago

Also, you should add a test that exercises this functionality and confirms that the code behaves as expected. Let me know if you need help with that too.

georgjaehnig commented 5 years ago

Hi, thanks for your offer to help. I've found the problem. The error

sqlite3.IntegrityError: UNIQUE constraint failed: notes.id

comes from 

deck1.add_note(note)
deck2.add_note(note)

in https://github.com/kerrickstaley/genanki/blob/master/tests/test_genanki.py#L147

The notes.id is generated by the timestamp and the note index note_idx within the deck. Since there are two different decks here, one created right after the other the notes.id is the same.

I only thought about notes being generated at the same time that come from the same deck – I didn't think about different decks.

So we would in fact need to track all the notes created, beyond deck instances. I'm not sure how to do this best.

Ideas for a solution that came to my mind:

It's all kind of messy because Anki puts ID and creation date into the same field. :(

georgjaehnig commented 5 years ago

So we would in fact need to track all the notes created, beyond deck instances. I'm not sure how to do this best.

Another idea: Before generating any sequence of notes (or cards):

This way, only at the beginning of the sequence we would read the db, the rest would run quickly.

Let me know what you think.

aalemayhu commented 3 years ago

@georgjaehnig Courtesy, ping. Is there any progress on this?

Thanks!

georgjaehnig commented 3 years ago

Nope, I didn't work on it after the last comment. From what I roughly remember it was rather an issue with the test than with the actual code having errors.

aalemayhu commented 3 years ago

I see, I tried running the tests locally and they failed with the below summary. So you are probably right, they need to be updated.

I also went ahead and tried to rebase against master from your branch. There were several merge conflicts, I am not familiar with the genanki code and ended up giving up 🙂 Is there any chance you could resolve the conflicts? I can then look into the tests, thank you 🙏🏾 

================================================================ short test summary info ================================================================
FAILED tests/test_genanki.py::TestWithCollection::test_multi_deck_package - sqlite3.IntegrityError: UNIQUE constraint failed: notes.id
FAILED tests/test_genanki.py::TestWithCollection::test_card_suspend - assert [1606578548000, 1606578548001] == [1, 2]
======================================================= 2 failed, 14 passed, 20 warnings in 1.34s =======================================================
georgjaehnig commented 3 years ago

OK, I have resolved the merge conflict. And generation also worked on one of my projects. Let's see if the checks will pass.

aalemayhu commented 3 years ago

@georgjaehnig I appreciate you following up 👍🏾 I have created a pull request with some fixes, targeting your master branch. Hopefully we can get the outstanding failures resolved 🙂

georgjaehnig commented 3 years ago

Thanks! I have just scrolled up and noticed this comment of mine:

The notes.id is generated by the timestamp and the note index note_idx within the deck. Since there are two different decks here, one created right after the other the notes.id is the same.

This means that we'd need a completely different approach, as is written in the comments below. One idea was this.

In the meantime, I also thought to maybe do it the hacky way with sleep()? Apparently, you can sleep also only milliseconds. I've added a line for testing but the tests still fail. :(

kerrickstaley commented 3 years ago

I fixed the note/card "Added" date issue (https://github.com/kerrickstaley/genanki/issues/29) in https://github.com/kerrickstaley/genanki/commit/f1c738ab59b216d304d753a4c86fb09bcbbbf37b, and that fix has been released in genanki 0.10.0 which is now on PyPI (https://pypi.org/project/genanki/0.10.0/). I think this PR is now obsolete, so I'm going to close it (but let me know if this is a mistake).

aalemayhu commented 3 years ago

I can confirm this works, thanks a lot 🙏🏾