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

WIP: Update to python 3 and anki 2.1 #46

Merged
merged 5 commits into from
Oct 28, 2018

Conversation

j-kramer
Copy link
Contributor

@j-kramer j-kramer commented Aug 23, 2018

I ported this project to Python 3 and Anki 2.1. All the test pass locally and I've been using this PR for about month without much of an issue. However there a still a few caveats, so feedback is appreciated.

  1. This PR was not tested on locales other than UTF-8.
  2. It is not a backwards compatible with Anki 2.0.
  3. My solution (kanjicolorizer/colorizer.py lines 34-43) to importing the argparser and colorsys dependencies, if kanjicolorizer is part of the Anki addon, is probably a hack. From what I understand the issue is that python 3+ does not support implicit relative imports anymore. My current solution catches the exception and imports them with an explicit relative import instead. I considered using the build system to patch the imports, but I'm not sure that is any better either. Maybe there is a better solution but my knowledge of python is rather limited.

In light of these issues how do we proceed on this PR?

edit: I forgot about cicleci: it uses python 2.7 instead of 3 so the tests fail.

@j-kramer j-kramer force-pushed the 2to3andaddon21 branch 7 times, most recently from 67b7224 to 525c104 Compare August 23, 2018 19:32
Anki requires python 3.6 or later. However this version is not available with cirrcleci 1.0, hence the migration to 2.0.
Sadly pytest-circleci 0.2 does not support python 3 yet, so parallel jobs are omitted for now.
@j-kramer j-kramer mentioned this pull request Aug 23, 2018
@cayennes
Copy link
Owner

Thanks! Sorry I'm so slow at getting to this.

  1. needs to be done - it was a while ago now, but my recollection is that the first version of this that I released was broken for many people because I failed to test on non-utf-8 locales. I'll try to get to this but it won't be within the next couple weeks, and if you could do it that would be great. Testing on all three major operating systems would also be good for significant changes like this.
  2. is to be expected; we can release two separate versions
  3. is a perfectly acceptable hack in the interest of getting this out

@j-kramer
Copy link
Contributor Author

I only have linux pc's, so I can't test the changes on the other operating systems. I did a quick test for non-utf-8 locales by starting anki with the C locale:

$ LANG=C anki
Traceback (most recent call last):
...
Exception: Anki requires a UTF-8 locale.

So at least for anki non-utf-8 locales are not an issue anymore. This leaves the doc-tests and unit-tests, which both pass with LANG=C.

@cayennes
Copy link
Owner

Thank you so much for this! I'm finally getting to it this week, and it's really great to have this pretty much done. I've successfully tested it on linux and windows so far, and I have just some small additions to make before I'll merge it to master and release it it.

@cayennes cayennes merged commit 97fda09 into cayennes:master Oct 28, 2018
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.

None yet

2 participants