Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

fix NoneType object is not iterable when iterate over self._waiters #727

Merged
merged 5 commits into from Nov 17, 2020
Merged

Conversation

jeffguorg
Copy link
Contributor

@jeffguorg jeffguorg commented Mar 30, 2020

What do these changes do?

#726 fix a problem lead by python3.8

Are there changes in behavior for the user?

nope

Related issue number

#726

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES/ folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example:
      Fix issue with non-ascii contents in doctest text files.

@jeffguorg
Copy link
Contributor Author

Hi there

this is a fix for #726. no changes in behavier for users. i just don't know if i should add a line or a file in CHANGES for this is my first time contributing to aioredis. CHANGES directory seems to be empty for now.

@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #727 (c65ce5e) into master (9e113e2) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
+ Coverage   96.92%   97.10%   +0.17%     
==========================================
  Files          57       57              
  Lines        8497     8114     -383     
  Branches      560      568       +8     
==========================================
- Hits         8236     7879     -357     
+ Misses        193      170      -23     
+ Partials       68       65       -3     
Impacted Files Coverage Δ
aioredis/locks.py 100.00% <100.00%> (ø)
aioredis/commands/streams.py 89.91% <0.00%> (-1.96%) ⬇️
tests/connection_commands_test.py 95.45% <0.00%> (-1.88%) ⬇️
aioredis/commands/server.py 82.46% <0.00%> (-1.64%) ⬇️
aioredis/sentinel/pool.py 81.67% <0.00%> (-0.32%) ⬇️
tests/integration_test.py 96.61% <0.00%> (-0.06%) ⬇️
aioredis/log.py 45.45% <0.00%> (ø)
aioredis/util.py 94.48% <0.00%> (ø)
tests/ssl_test.py 100.00% <0.00%> (ø)
... and 39 more

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 9e113e2...d0d34ce. Read the comment docs.

@Andrew-Chen-Wang
Copy link
Collaborator

@jeffguorg It seems like the linters got ya. Install flake8 and run flake8 * for your python files. It should pass the test after that.

@jeffguorg
Copy link
Contributor Author

@Andrew-Chen-Wang

Hi, thanks. and i updated my pr. but i see some checks still fails after that. does that matter?

@Andrew-Chen-Wang
Copy link
Collaborator

@jeffguorg Yes. Take a look below these comments, and you'll see all the CI tests. Press "Details" for the Travis CI one and check each traceback error when you load the log.

@jeffguorg
Copy link
Contributor Author

@Andrew-Chen-Wang Hi, thanks for your guidance and sorry i was busy working for some other issues.

I see that there are two issues.

one issue is about timeline protection. but there is no files in CHANGES directory and CHANGES.txt was not updated for months. where should i put the description?

another is about tests against test/*.py. one of the test failed. the line looks like this:
E assert 44087 != 44087
but i didn't find that line in the tests

@Andrew-Chen-Wang
Copy link
Collaborator

The line is tests/sentinel_failover_test.py:55: AssertionError.

You'll see the lines of failure as red and right below the printed code. Don't worry about timeline protection as that's not required.

@jeffguorg
Copy link
Contributor Author

@Andrew-Chen-Wang

sorry I was not responding for a period.

yes. I see that the test case is located in tests/sentinel_failover_test.py:55. I misunderstood the output. the assertion line is assert master.address[1] != old_port.

but I have no clue why it is failing. nothing about cluster failover had been changed. is it possible that the test case has flaws?

@Andrew-Chen-Wang
Copy link
Collaborator

@jeffguorg Not a problem. It's open source volunteering after all :)

Can you try running the test case on your local machine? You can try it using this documentation: https://aioredis.readthedocs.io/en/v1.3.0/devel.html or perhaps just run python setup.py. After that, it's just a lot of print() statements and figuring out where it's going wrong.

You've only taken a look at that single travis build. There are other Travis builds that have other issues. Although I'm not entirely sure how Python 3.9 got in there... perhaps it's not something this PR should be concerned over.

Copy link

@CatCanCreate CatCanCreate left a comment

Choose a reason for hiding this comment

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

LGTM!

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2020

CLA assistant check
All committers have signed the CLA.

@seandstewart seandstewart linked an issue Nov 14, 2020 that may be closed by this pull request
@seandstewart
Copy link
Collaborator

@jeffguorg -

We can ignore the CI failures for now. Once you've followed the checklist (be sure to add a file to CHANGES and add yourself as a contributor!) we can get this merged.

@jeffguorg
Copy link
Contributor Author

@seandstewart
Hi, thanks for your guidance

this is my first time committing to a public repo. i added CHANGES/726.bugfix. is it good enough to merge into the repo?

@seandstewart
Copy link
Collaborator

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ seandstewart
❌ Chao Guo
Chao Guo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jeffguorg looks like you still need to sign the CLA

@jeffguorg
Copy link
Contributor Author

@seandstewart done!

I've signed the cla for long. it turns out that I was coding at my office so the commit author was wrong. the bot didn't recognize it. lol

@seandstewart seandstewart merged commit d974e33 into aio-libs-abandoned:master Nov 17, 2020
@evgeniikozhanov
Copy link

When the next release will possible?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python3.8 Support: change of asyncio.locks.Lock
6 participants