Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue #142, Lots of deprecated items when starting anki with crowdanki #151

Merged
merged 1 commit into from
Nov 28, 2021

Conversation

evandrocoan
Copy link
Contributor

fix #142, Lots of deprecated items when starting anki with crowdanki

@aplaice
Copy link
Collaborator

aplaice commented Nov 25, 2021

Thanks very much! The PR successfully removes the deprecations warning messages for Anki 2.1.47/2.1.48/2.1.49 (unfortunately, for the beta 2.1.50 there are more deprecation warnings, at least partially due to the migration to Qt6, but that's out-of-scope for this PR), while maintaining compatibility with earlier versions (2.1.40+).

Please add some message to the effect of "TODO Remove compatibility shims for Anki 2.1.46 and lower." before each of the hasattrs, though.


I'm not sure whether this is the cleanest approach of maintaining compatibility*, but no alternative is exactly great, and we still don't have a policy for deciding how, when and for how long to maintain compatibility with older versions. None of this is obviously an obstacle to merging this PR.

* My main worry is about added complexity (this PR consists of tiny changes, but eventually tiny changes add up, making it harder to reason about the logic of Anki's and CrowdAnki's internals).

Stvad#142
Lots of deprecated items when starting anki with crowdanki
@aplaice aplaice merged commit 2e1b08e into Stvad:master Nov 28, 2021
@evandrocoan evandrocoan deleted the fix_warnings branch November 28, 2021 22:40
@evandrocoan
Copy link
Contributor Author

Hi. Sorry, I did not see your message from 3 days ago. I am not sure why I missed it.

To keep the code cleaner I could use the anki_version = anki_version.split(".") thing in other places instead of hasattr.

About the qt6 warnings, I reverted back to the old version of Anki (with qt5) because there is a problem with this new version of Anki. It is striping <script> tags from fields already in my collection. I will stick with qt5 until I decide to fix it on Anki code or give up and remove <script> tags from my fields.

Also, qt6 makes my Hiragana/Kanji font look quite ugly/bold. Another thing I will have to take time to do something about. But not for now at least.

I did this pull-request because when I would export my collection, I would see 5.000 of those warnings and I could miss something important in there. I could actually, instead of changing the CrowdAnki code, just disable the warning on the Anki code (as I run it from my own fork). But I only got this idea after I opened this pull-request.

My main worry is about added complexity

I would not worry much about the complexity of the hasattr thing (@dae is pushing to rename functions to snake_case). But the anki_version = anki_version.split(".") it worries me because someday @dae may do something like 2.1.33-555 or something else other than the main format.

If the worries would be about performance, we could run a benchmark with cprofile module from python, but I think today there are other things more troublesome for performance. Today takes about 10 seconds to export my collection (I think it is reasonable). But this is only because I disabled the git management code and the audio exporting, otherwise, it would take about 3 minutes to export my collection (5000 notes, 25.0000 audios on my old hard disk).

@aplaice
Copy link
Collaborator

aplaice commented Nov 29, 2021

No worries; thanks for replying!

I greatly appreciate that you opened a pull-request rather than just making a private change to Anki or CrowdAnki.

My main complexity worry is long-term — that after several waves of adding compatibility shims, we'll have a tree of various combinations (if x ... else ... or if version < ...), which will clutter up the code, make it harder to reason about what the code is doing etc.

I've added the annotations, so that it'll be clear which versions of Anki the shims are necessary for, in the future, so I don't think there's any need for further changes.

Yeah, I wouldn't worry about performance regressions, here — the changes won't have a sizable relative performance impact. (In the medium term, I'd obviously like to improve performance, overall...)

Thanks again for the PR! :)


Aside: I'd recommend reporting the issues with Qt6 Anki!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lots of deprecated items when starting anki with crowdanki
2 participants