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

Fix health check after unsubscribe #1207

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

Conversation

Andrew-Chen-Wang
Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang commented Nov 17, 2021

What do these changes do?

Returns None for health check response.

Are there changes in behavior for the user?

Maybe. If someone used to check the health check response after unsubscribing (for some odd reason), then yes. But for most people, no. And I can't imagine why since we usually return None for health checks.

Related issue number

Fixes #1206

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.

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #1207 (5e0ceea) into master (a708bd1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5e0ceea differs from pull request most recent head d4fe77c. Consider uploading reports for the commit d4fe77c to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1207   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files          21       21           
  Lines        6872     6873    +1     
  Branches      794      794           
=======================================
+ Hits         6131     6132    +1     
  Misses        578      578           
  Partials      163      163           
Flag Coverage Δ
unit 89.13% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aioredis/client.py 82.56% <100.00%> (+<0.01%) ⬆️

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 a708bd1...d4fe77c. Read the comment docs.

@bmerry
Copy link
Collaborator

bmerry commented Nov 22, 2021

The code LGTM for this specific issue, but it is probably worth adding a comment explaining why you're checking for two possible responses.

aioredis/client.py Outdated Show resolved Hide resolved
Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>
@Andrew-Chen-Wang
Copy link
Collaborator Author

not sure what's going on. Will come back later if no one else figures out what's wrong. Branch should be open for anyone to change...

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