-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement part of Github direct upload method #137
base: master
Are you sure you want to change the base?
Conversation
Hi @shaunakg, thanks for contributing this! re PyGithub - it's ok, as long as it does not transitively pull in dependency on native things (e.g. native git binary?)@aplaice I'd appreciate you taking a look as-well if you have a chance! |
This would be really convenient to have, so thanks very much for writing it! Unfortunately, I haven't had time to look at or test it.
On a very cursory glance, it seems that |
If it is bringing in a binary dependency - I don't know of a way to properly package it into the anki plugin such that it'd work properly on all platforms 🙁 |
By "all platforms" do you mean mobile platforms also? In that case you can enable the feature only in Desktop platforms and disable it for the mobile ones. |
No. The issue is that it seems that PyNaCl requires a compiled C component. The maintainers of PyNaCl have uploaded binary wheels to pypi, so that when installing with pip (or pipenv etc.), pip simply downloads the wheels from pypi, appropriate for the given platform (Windows, Linux, MacOS, etc.) and the given processor type (x86, arm(32|64) etc.), provided these wheels are available, so there's little issue. However, an Anki add-on has to be self-contained, so for PyNaCl to function we'd have to include all the many different binary wheels and use the correct ones at run-time. The problems are:
(Anki add-ons aren't available on mobile (iOS or Android) at all, so mobile Anki isn't the issue.) |
Thank you for the detailed reply. I'll try testing this on my machine but I remember a similar case of XGBoost where the binaries for all target platforms were included in the final package. I'll test and come back to you later. |
This adds some methods into the exporter class and export wrapper class to facilitate the direct export of the JSON representation into Github. It also adds some config entry fields where the user can put their Github personal access token, username and repository name.
This is not ready for merging yet because it does not actually show up in the UI as an export option. I am working on doing this but it will likely require the rewriting of part of the export dialog.
It includes the dependency
PyGithub
because it is much easier to use the API of Github over natively implementing Git through preexisting dependencydulwich
and pushing the files. This might be able to be done but would be much more complicated (in my opinion).