KeenClient-Python icon indicating copy to clipboard operation
KeenClient-Python copied to clipboard

rm pycryptodome

Open BlackVegetable opened this issue 7 years ago • 3 comments

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

BlackVegetable avatar Oct 23 '17 18:10 BlackVegetable

Neat. I didn't know about reraise.

On Oct 25, 2017 6:33 PM, "Justin Mason" [email protected] wrote:

@masojus approved this pull request.

Minor comment, but looks fine.

In keen/scoped_keys.py https://github.com/keenlabs/KeenClient-Python/pull/141#discussion_r147022171 :

+try:

  • from Crypto.Cipher import AES +except ImportError as ie:
  • 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

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 ... facility in python 3. Just a thought.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/keenlabs/KeenClient-Python/pull/141#pullrequestreview-72052203, or mute the thread https://github.com/notifications/unsubscribe-auth/ACitbywOx3fECfFiv7-BAJdED8O5_DMmks5sv9NLgaJpZM4QDU1s .

BlackVegetable avatar Oct 26 '17 01:10 BlackVegetable

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.

BlackVegetable avatar Oct 31 '17 14:10 BlackVegetable

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.

masojus avatar Mar 20 '19 08:03 masojus