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.

Sign up for GitHub

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

Jump to bottom

Remove `_ex_keys` mapping #128

Merged

bmerry merged 1 commit into jamesls:master from ticosax:py3-fix

Mar 22, 2018

Contributor

ticosax commented Sep 20, 2016 •

edited

Because they can get out of sync under python3 since
_dict entries are converted into bytes while _ex_keys aren't
and b'foo' != 'foo' under python 3.

Instead of trying to convert back and forth between bytes and str in different places,
this new implementation stores everything in _dict and
makes sure values and timestamp are always stored together.

coveralls commented Sep 20, 2016

Coverage decreased (-0.02%) to 98.132% when pulling 0811f2e on ticosax:py3-fix into 4bfc7a2 on jamesls:master.

4 similar comments

coveralls commented Sep 20, 2016

Coverage decreased (-0.02%) to 98.132% when pulling 0811f2e on ticosax:py3-fix into 4bfc7a2 on jamesls:master.

coveralls commented Sep 20, 2016

Coverage decreased (-0.02%) to 98.132% when pulling 0811f2e on ticosax:py3-fix into 4bfc7a2 on jamesls:master.

coveralls commented Sep 20, 2016

Coverage decreased (-0.02%) to 98.132% when pulling 0811f2e on ticosax:py3-fix into 4bfc7a2 on jamesls:master.

coveralls commented Sep 20, 2016

Coverage decreased (-0.02%) to 98.132% when pulling 0811f2e on ticosax:py3-fix into 4bfc7a2 on jamesls:master.

Owner

jamesls commented Sep 22, 2016

Thanks for the pull request.

Per the contributing guidelines, can you add a test that demonstrates the bug? That way we can verify this fixes the issue and prevent regressions in the future. Thanks.

jamesls added the NeedsTests label

Contributor Author

ticosax commented Sep 26, 2016

The fix is deprecating any test I could provide since I'm removing _ex_keys. and the bug exists because _ex_keys exists. To be honest I started by writing a test, try to fix the bug, then realize it was easier to fix by removing _ex_keys and the test I wrote didn't make sense anymore.
What do you suggest ?

Contributor

jdufresne commented Jan 28, 2017

I think the test, test_set_existing_key_persists(), added in PR #137 would be one that demonstrates the improvement.

Contributor

blueyed commented Jun 26, 2017

@ticosax
Needs to get rebased also.


          Remove _ex_keys mapping

e37577d

Because they can get out of sync under python3 since
`_dict` entries are converted into bytes while `_ex_keys` aren't
and ```b'foo' != 'foo'``` under python 3.

Instead of trying to convert back and forth between `bytes` and `str`
in different places, this new implementation stores everything
in `_dict` and makes sure values and timestamp are always stored together.

ticosax force-pushed the py3-fix branch from 0811f2e to e37577d Compare

June 27, 2017 08:57

Contributor Author

ticosax commented Jun 27, 2017

thank you @blueyed for the reminder.

Collaborator

bmerry commented Nov 10, 2017

I've created a fork called fakenewsredis with a PyPI release to incorporate PRs that aren't getting traction here. I've included this PR.

bmerry mentioned this pull request

Check keys exist before delete in _StrKeyDict._update_expired_keys #118

Closed

bmerry merged commit e37577d into jamesls:master

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