KeenClient-Python
KeenClient-Python copied to clipboard
keenlabs
pycryptodome as optional requirement
The cryptography library used (pycryptodome) is not pure python. As such, it requires a C compiler, and can also create problems running WSGI apps unless the WSGIApplicationGroup is set properly. It looks like this dependency is only required for scoped keys, an optional feature that is not required for all use cases.
Would it make sense to let this be optional and throw an exception if somebody tries to use scoped keys without this dependency installed? I am happy to make a pull request but just wanted to make sure this would make sense first.
I'm inclined to want to include it in requirements.txt, but provide some other requirements-like file that could be used to install dependencies for those not running systems with a C compiler. Then changes would need to be made to only import the pycryptodome library if scoped keys were being run, but neither change would be hard. What are your thoughts?
In general, people will want to be able to pip install either version. As such, would it make sense to add an extras keyword to setup.py for the scoped keys feature? As such, one could pip install keen to install without pycryptodome and pip install keen[signed-keys] to also include requirements required for signed keys?
If you can think of a way to make the default install include pycryptodome and the alternative to not install it, that'd be better. I worry about making the common-case not feature complete.
Ah, I see what you are saying, makes sense. I don't think that pip has any native features allowing for dependency exclusion, let me look into some alternatives.
I actually think having the base SDK not have scoped key features is fine. As long as we call it out in docs.
Ah, then that simplifies things. FWIW, I'm going to make a bunch of changes to this codebase in the very near future, so I wouldn't go writing a patch/PR for this just yet.
Sorry for the delay on this. I'm still working through 1 PR and another branch soon-to-become a PR. If you want to write up a patch here, we can just deal with the merge conflicts later rather than forcing you to wait for my timetable.
Given that we've deprecated Scoped Keys, I think I'll completely drop this requirement for the next version and note that the library must be manually installed in the section that talks about the (deprecated) Scoped Keys feature.