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

Response parsing occasionally fails to parse floats #1692

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Nov 10, 2021

  • filtering out codecov paths
  • parse_to_list fails to parse float values

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please provide a description of the change here.

@chayim chayim added bug Bug breakingchange API or Breaking Change labels Nov 10, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2021

Codecov Report

Merging #1692 (008f8f8) into master (939fead) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1692      +/-   ##
==========================================
+ Coverage   92.05%   92.22%   +0.17%     
==========================================
  Files          52       53       +1     
  Lines       10973    11003      +30     
==========================================
+ Hits        10101    10148      +47     
+ Misses        872      855      -17     
Impacted Files Coverage Δ
redis/commands/helpers.py 91.30% <100.00%> (+33.16%) ⬆️
tests/test_helpers.py 100.00% <100.00%> (ø)
tests/test_search.py 98.58% <0.00%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 939fead...008f8f8. Read the comment docs.

@chayim
Copy link
Contributor Author

chayim commented Nov 10, 2021

@bmerry Tagging you here given our other bug. Is there any way I can test this specific change with your project? I'm changing a helper underneath, but I realize it could expose a bug, if people assume that floats are strings. If not - I'm merge ready.

@bmerry
Copy link
Contributor

bmerry commented Nov 10, 2021

I'm afraid I'm lacking some context. What is parse_to_list used for?

@bmerry
Copy link
Contributor

bmerry commented Nov 10, 2021

Is there any way I can test this specific change with your project?

The project is fakeredis, which implements a Connection subclass that pretends to be a Redis server. It hasn't kept up to date with all the new Redis protocol things, which is why tests have been failing when redis-py generates such protocol.

You can probably try running the fakeredis unit tests yourself. You should run them against a Redis 6.2.6 server (otherwise you'll get a bunch of failures for the stuff where it does emulate that version), and be aware that it will discard any data currently held by the server. Alternatively I think it'll skip the tests against the real redis server if it can't connect, and that'll make the tests twice as fast to run too.

@chayim
Copy link
Contributor Author

chayim commented Nov 10, 2021

I'm afraid I'm lacking some context. What is parse_to_list used for?

I realise that this isn't something that you'd currently be using. It's part of the 4.0.0 module support. Apologies, I shouldn't have tagged you.

On the plus side, I've closed fakeredis, which is pretty fun. Thanks :D!

@chayim chayim merged commit e07bd94 into master Nov 10, 2021
@chayim chayim deleted the ck-helpers branch November 10, 2021 13:09
@chayim chayim removed the breakingchange API or Breaking Change label Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants