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.

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

Sync request() and ws_connect() #2295

Merged
merged 2 commits into from
Sep 29, 2017
Merged

Sync request() and ws_connect() #2295

merged 2 commits into from
Sep 29, 2017

Conversation

asvetlov
Copy link
Member

Fix for #2292

@codecov-io
Copy link

codecov-io commented Sep 28, 2017 •
edited
Loading

Codecov Report

Merging #2295 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2295   +/-   ##
=======================================
  Coverage   97.24%   97.24%           
=======================================
  Files          39       39           
  Lines        8201     8201           
  Branches     1439     1439           
=======================================
  Hits         7975     7975           
  Misses         98       98           
  Partials      128      128
Impacted Files Coverage Δ
tests/autobahn/client.py 94.72% <0%> (ø) ⬆️

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 716fcaa...13c180d. Read the comment docs.

@asvetlov
Copy link
Member Author

Any review?
If not -- I'll merge it tomorrow evening.

@cecton
Copy link
Contributor

cecton commented Sep 28, 2017

Actually I don't check every day that there is new PR so it's a good thing you added me. I'm reviewing now

cecton
cecton approved these changes Sep 28, 2017
View reviewed changes
Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

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

:shipit:

docs/client_reference.rst

Add *auth* parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intentionally removed part of the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.
I've dropped versioadded records for versions < 2.0
They don't carry any useful information now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup it was quite noisy after all.

@cecton
Copy link
Contributor

cecton commented Sep 29, 2017

You could also rebase instead of merging the master branch to the feature branch, it will prevent having merge commits in the tree that doesn't carry any useful information.

@cecton
Copy link
Contributor

cecton commented Sep 29, 2017

Ahhh I see! That's why you use the squash and merge button, it removes the merge commit at the same time.

@asvetlov
Copy link
Member Author

Yes, with squashing I don't care about commits history

@asvetlov asvetlov merged commit 17cd3e9 into master Sep 29, 2017
@asvetlov asvetlov deleted the sync-request-and-ws_connect branch September 29, 2017 10:32
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cecton cecton cecton approved these changes

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asvetlov @codecov-io @cecton