Skip to content

Conversation

DrWacker
Copy link
Contributor

headers are added to the websockets connect method
keep alive message is handled without crashing

What kind of change does this PR introduce?

This feature allows the passing of header information in the subscribe method. Also it handles a keep alive message

What is the current behavior?

#28

What is the new behavior?

See changes comment above

Does this PR introduce a breaking change?

Nope

Other information

N/A

headers are added to the websockets connect method
keep alive message is handled without crashing
@xkludge
Copy link
Contributor

xkludge commented Oct 12, 2020

Thanks @DrWacker, could you run black on the project and add a test for the functionality you added. Thanks!

@DaleSeo
Copy link
Contributor

DaleSeo commented Oct 12, 2020

@DrWacker Regarding the formatting issue, please refer to the following contributing guide. Pre-commit hook will automatically format your code changes.
https://github.com/prodigyeducation/python-graphql-client/blob/master/docs/CONTRIBUTING.md#getting-started

It would be great if you could write unit tests for the bug fix but it wouldn't be necessary.

@DrWacker
Copy link
Contributor Author

DrWacker commented Oct 12, 2020 via email

@DaleSeo DaleSeo linked an issue Oct 13, 2020 that may be closed by this pull request
@@ -3,7 +3,7 @@ repos:
rev: stable
hooks:
- id: black
language_version: python3.7
language_version: python3.8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python3.8 is required for IsolatedAsyncioTestCase so I changed it here as well

@DrWacker
Copy link
Contributor Author

DrWacker commented Oct 13, 2020

@DaleSeo @xkludge I don't know why black is failing in the pipeline. I'm getting green on my screen. Do either of you have any debugging tips?

> git commit -am "added unit test"
black....................................................................Passed
flake8...................................................................Passed
test.....................................................................Passed
[subscription_headers 4e7666a] added unit test
2 files changed, 60 insertions(+), 1 deletion(-)

Nevermind. Just ran black manually.

@DaleSeo
Copy link
Contributor

DaleSeo commented Oct 13, 2020

@DrWacker It seems like there's a bit of discrepancy between our tooling. I tried to run Black locally. Please refer to the following feedback from Black.

image

@DaleSeo DaleSeo self-requested a review October 13, 2020 15:53
@DrWacker
Copy link
Contributor Author

@DaleSeo if I'm looking at your output correctly my most recent push now matches that.

Copy link
Contributor

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

@DrWacker Thanks for fixing formatting issues and adding unit tests! The code looks great!

@DaleSeo DaleSeo merged commit 268e1f7 into prodigyeducation:master Oct 13, 2020
@DrWacker
Copy link
Contributor Author

@xkludge @DaleSeo Thanks for your help. What's needed to push these changes to pypi.org? If nothing needs to be done what's the usual turnaround time for that to happen?

@DaleSeo
Copy link
Contributor

DaleSeo commented Oct 13, 2020

@DrWacker I'll create a PR to bump the version now. Once it gets merged, the new version will show up in Pypi in a couple of minutes or so.

@DrWacker
Copy link
Contributor Author

@DaleSeo Thanks!

@DaleSeo DaleSeo mentioned this pull request Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header ignored in GraphqlClient.subscribe
3 participants