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

#145: Cleanup TCP configurations across platforms and unified defaults into one dict #167

Merged
merged 2 commits into from
Oct 23, 2017

Conversation

darkdreamingdan
Copy link
Contributor

This commit is meant to fix #145 , but also goes further to refactor the default TCP settings which were scattered in different places throughout the code. Now, TCP settings are clearly defined within platform.py, and the correct TCP settings are declared here by moving the KNOWN_TCP_OPTS. There are no longer HAS_TCP_* settings which led to fairly confusing code.

#145 was caused by WSL not supporting the majority of TCP platforms, but Python's socket library making them available because it thinks it's a standard Linux platform.

@codecov-io
Copy link

codecov-io commented Sep 16, 2017

Codecov Report

Merging #167 into master will increase coverage by 0.23%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage    91.6%   91.84%   +0.23%     
==========================================
  Files          14       14              
  Lines        1668     1668              
  Branches      232      235       +3     
==========================================
+ Hits         1528     1532       +4     
+ Misses        111      108       -3     
+ Partials       29       28       -1
Impacted Files Coverage Δ
amqp/platform.py 72.5% <85.71%> (+5.83%) ⬆️
amqp/transport.py 69.73% <87.5%> (+0.64%) ⬆️

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 ae05e31...a98c960. Read the comment docs.

@darkdreamingdan
Copy link
Contributor Author

I don't believe the above errors are due to my code changes.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

could you also verify the proposed changes by adding tests?

@darkdreamingdan
Copy link
Contributor Author

I've added some tests around this. Once again - I don't think I'm responsible for failing tests.

@darkdreamingdan
Copy link
Contributor Author

Are there any other requirements to be fulfilled before this can be merged?

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.

AMQP 2.1.2<= fails on Bash On Windows with OperationError: [Errno 92] Protocol not available
3 participants