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

rm pycryptodome #141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

rm pycryptodome #141

wants to merge 2 commits into from

Conversation

BlackVegetable
Copy link
Contributor

Scoped Keys are deprecated and pycryptodome is not pure python
making it a problem for people wanting to pip install keen on
some systems. pycryptodome is a dependency of only scoped keys.

To make the tests pass, you'll need to have it installed but that
is enabled via the test_requires within setup.py. A fun error
message is displayed when anything scoped keys are imported. This
may make from keen import * fail, but that's bad python to begin
with so I don't feel very bad about that.

This includes a documentation update reflecting the manual step
required for the deprecated feature.

Testing

Try running the test suite without pycryptodome installed. It should still succeed.
Try import keen.scoped_keys without pycryptodome installed. It should fail with a message.

Related tickets

#131

Scoped Keys are deprecated and pycryptodome is not pure python
making it a problem for people wanting to `pip install keen` on
some systems. pycryptodome is a dependency of only scoped keys.

To make the tests pass, you'll need to have it installed but that
is enabled via the test_requires within setup.py. A fun error
message is displayed when anything scoped keys are imported. This
may make `from keen import *` fail, but that's bad python to begin
with so I don't feel very bad about that.

This includes a documentation update reflecting the manual step
required for the deprecated feature.
@BlackVegetable BlackVegetable self-assigned this Oct 23, 2017
Copy link
Contributor

@masojus masojus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but looks fine.

ie.args = (ie.args[0] + ' -- Scoped Keys are deprecated in favor'
' of Access Keys. To use Scoped Keys anyway, run: '
'`pip install pycryptodome` and try this again.',)
raise ie
Copy link
Contributor

@masojus masojus Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will blow away the original stack, right? raise ie in python 2 will blow away the stack anyway since you have to use the 3-arg raise, and in python 3, by replacing the entire args tuple we're disregarding any information previously stored on the exception except the message, right? In this case, maybe the message is enough, but if not, we could use the six module's reraise which I think forwards to the future utils in python 2 and uses the raise ... from ... and/or the with_traceback() facility in python 3. Just a thought.

@BlackVegetable
Copy link
Contributor Author

BlackVegetable commented Oct 26, 2017 via email

@BlackVegetable
Copy link
Contributor Author

I'd like to give 0.5.1 a chance to simmer in the event that I have something broken still and need to push 0.5.2 rather than make progress on 0.6.0. In particular, I'll want to get better test coverage overall before I'll be satisfied with 0.5.x moving onward. So I'm holding off, at least for the moment, on merging this.

@masojus
Copy link
Contributor

masojus commented Mar 20, 2019

I am thinking I can also use environment markers and extras_require here to pick and choose the dependencies in setup.py. That doesn't mean we don't need the runtime checks, but could make some things easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants