-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add mtherieau's Cloze code to genanki #37
Conversation
genanki/note.py
Outdated
if op(self.fields[ord_] for ord_ in required_field_ords): | ||
rv.append(Card(card_ord)) | ||
return rv | ||
if self.model.model_type == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you define separate _front_back_cards()
and _cloze_cards()
methods so that you can keep each method shorter and more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely easier to read now that it is addressed with 4aff1c6
genanki/note.py
Outdated
rv.append(Card(card_ord)) | ||
return rv | ||
# returns a Card with unique ord for each unique cloze reference | ||
assert self.model.model_type == 1, self.model.model_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an assertion, I would use an if-statement and throw an exception if model_type
is not 0
or 1
.
Assertions aren't really intended for checking the validity of arguments that someone else passed to you (coming from code that you didn't write). They're more intended for checking the internal validity of your own code.
Regular exceptions are better because you can throw an specific type of exception (assert
can only raise AssertionError
) and because they are always run (assert
statements are disabled when running in "optimized" mode).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 7d59da9
assert self.model.model_type == 1, self.model.model_type | ||
card_ords = set() | ||
# find cloze replacements in first template's qfmt, e.g "{{cloze::Text}}" | ||
cloze_replacements = set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a regex it may be better to look at Anki's code to see how it does it (probably uses a real template engine to parse the template and some sort of trick to figure out what the cards should be; see _req
in this file for a similar thing). Regexes can be fragile and hard to debug. And also hard to review.
That said, I'm OK with shipping this as a v1 and waiting for the bug reports to roll in :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a quick note that although I agree that regexes can be fragile and difficult to debug, these regexes were actually lifted from anki: https://github.com/dae/anki/blob/eef86bf37ebf8c4f9eb4cee50b10aedecfe647aa/anki/models.py#L578
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtherieau's regex exactly matches anki's code, so I think this is okay as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
tests/test_cloze.py
Outdated
@@ -0,0 +1,70 @@ | |||
#!/usr/bin/env python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing tests!
Could you remove the #!/usr/bin/env python
and unset the executable bit? I think it may cause confusion for people who just want to run the test. If you want to execute this file you can always do it by running python3 tests/test_cloze.py
.
Also side note: you shouldn't just put python
in a shebang; you should always specify python2
or python3
unless it's compatible with both. There is a PEP that I'm very familiar with that explains why :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link to the PEP. Great information.
The top line is removed and the exe bit is unset with 2f18899
tests/test_cloze.py
Outdated
|
||
fields = ['NOTE TWO: {{c1::1st deletion}} {{c2::2nd deletion}} {{c3::3rd deletion}}', ''] | ||
my_cloze_note = Note(model=MY_CLOZE_MODEL, fields=fields) | ||
assert {card.ord for card in my_cloze_note.cards} == {0, 1, 2} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to assert the sorted list is equal, i.e.
assert sorted(card.ord for card in my_cloze_note.cards) == [0, 1, 2]
Otherwise there could be duplicate ord
values in my_cloze_note.cards
and this test wouldn't catch them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that. Thanks. This is addressed with: c61a8bc
for note in notes: | ||
deck.add_note(note) | ||
fout_anki = '{NAME}.apkg'.format(NAME=deckname) | ||
Package(deck).write_to_file(fout_anki) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you avoid writing this file and printing to the console by default? People won't want this behavior when they're running tests.
You could put the behavior behind a flag, e.g. add a write_to_test_apkg=False
flag and put if write_to_test_apkg
right before deckname = 'mtherieau'
, and then run test_cloze(True)
in your if __name__ == '__main__':
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 03523a4
genanki/note.py
Outdated
field_index = next((i for i, f in enumerate(self.model.fields) if f['name'] == field_name), -1) | ||
field_value = self.fields[field_index] if field_index >= 0 else "" | ||
# update card_ords with each cloze reference N, e.g. "{{cN::...}}" | ||
card_ords.update([int(m)-1 for m in re.findall(r"{{c(\d+)::.+?}}", field_value) if int(m) > 0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You can remove the []
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 0fa99f6
assert {card.ord for card in my_cloze_note.cards} == {0, 1, 2} | ||
notes.append(my_cloze_note) | ||
|
||
fields = ['NOTE THREE: {{c1::1st deletion::C1-CLOZE}}', ''] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add another test case where there are multiple Cloze deletions with the same index, e.g.
'NOTE FOUR: {{c1:1st deletion}} foo {{c2:2nd deletion}} bar {{c1::3rd deletion}}'
This is documented in the manual:
You can also elide multiple sections on the same card. In the above example, if you change c2 to c1, only one card would be created, with both Canberra and 1913 hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I am working on it...
Thank you for considering the pull request and thank you for the useful suggestions. I will take a look in a little bit as I am under a deadline right now. |
genanki/note.py
Outdated
def _cloze_cards(self): | ||
"""returns a Card with unique ord for each unique cloze reference""" | ||
if self.model.model_type != self.model.CLOZE: | ||
raise RuntimeError('Expected model_type CLOZE or FRONT_BACK') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueError
would be better than RuntimeError
here. That's the preferred exception to use when an argument passed to a function does not have a valid value.
For example, Python raises a ValueError
when you try to do math.sqrt(-1)
:
In [1]: import math
In [2]: math.sqrt(-1)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-2-2037b8d41d70> in <module>
----> 1 math.sqrt(-1)
ValueError: math domain error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done: #37 (comment)
Thank you for merging this. Is there anything else I need to do for completeness? Work sidelined me for a while. Thank you for your code review and comments and adding to that last repeated field test. Your comments were constructive. If anything is else is needed for this PR, I have time to work on it tomorrow. |
@dvklopfenstein sorry for the delay. I've published your changes to PyPI. Version 0.8.0 of genanki has your changes. |
#3
Please consider this pull request which adds support for Cloze cards to genanki.
I added the code from @mtherieau's gist example, as it works great.
Thank you creating this repo and sharing it with everyone and thank you for taking the time to consider this pull request.