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

Add support for multiple addresses when publishing a service #170

Merged
merged 5 commits into from May 22, 2019

Conversation

Projects
None yet
5 participants
@dtantsur
Copy link

commented May 20, 2019

This is a rebased and fixed version of PR #27, which also adds compatibility shim for ServiceInfo.address and does a proper deprecation for it.

@dtantsur dtantsur force-pushed the dtantsur:multi-addr branch from 8151947 to 815ce3d May 20, 2019

@coveralls

This comment has been minimized.

Copy link

commented May 20, 2019

Coverage Status

Coverage increased (+0.04%) to 93.644% when pulling 90f70f0 on dtantsur:multi-addr into 12477c9 on jstasiak:master.

dtantsur added some commits May 20, 2019

Provide proper deprecation of the "address" argument and field
* Raise deprecation warnings when address is used
* Add a compatibility property to avoid breaking existing code
  (based on suggestion by Bas Stottelaar in PR #27)
* Make addresses keyword-only, so that address can be eventually
  removed and replaced with it without breaking consumers
* Raise TypeError instead of an assertion on conflicting address
  and addresses
Disable black on ServiceInfo.__init__ until black is fixed
Due to python/black#759 black produces
code that is invalid Python 3.5 syntax even with --target-version py35.
This patch disables reformatting for this call (it doesn't seem to be
possible per line) until it's fixed.

@dtantsur dtantsur force-pushed the dtantsur:multi-addr branch from 815ce3d to 90f70f0 May 20, 2019

@jstasiak

This comment has been minimized.

Copy link
Owner

commented May 22, 2019

Nice!

@jstasiak jstasiak merged commit c787610 into jstasiak:master May 22, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 93.644%
Details
@jstasiak

This comment has been minimized.

Copy link
Owner

commented May 22, 2019

I'm thinking – maybe it makes sense to keep address working as long as len(addresses) == 1 and (in the future) make it throw an exception when there's more than a single address.

@dtantsur

This comment has been minimized.

Copy link
Author

commented May 22, 2019

I'm on the fence about this. I don't think writing info.addresses[0] is so much harder than info.address and having two ways to do the same thing may be confusing. I'm worried about people just writing info.address without realizing that it won't always work.

@jstasiak

This comment has been minimized.

Copy link
Owner

commented May 22, 2019

Yeah, you're probably right.

@balloob

This comment has been minimized.

Copy link

commented May 31, 2019

Could you make a new release? This is actually fixing a bug where a ServiceInfo without an address would hit an assertion error.

@jstasiak

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

@balloob Sure, version 0.23.0 has just been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.