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

Removed externalPort assignment when not set #1378

Merged
merged 3 commits into from May 12, 2020

Conversation

pramodhkp
Copy link
Contributor

I went ahead and created a PR that fixes #1360. We needed it to fix something on our side.

Let me know if it makes sense/ if there are any other changes that's required to get it merged.

@pramodhkp
Copy link
Contributor Author

Hey @oberstet , were you able to give this some thought?

@oberstet
Copy link
Contributor

hey, thanks for contributing!

ok, so I had a closer look .. couple of comments below. if you have a chance to modify the PR, I'd merge. this is a deeply buried, subtle change, but I think this should be good.

  1. we can simplify the code

by replacing this whole block with just

self.externalPort = externalPort
  1. we should change the comment
# do port checking only if externalPort or URL was set

to

# do port checking only if externalPort was set
  1. and finally, I think it also makes sense to remove the else block here


all of this should render the external port checking optional, and only apply when externalPort was indeed set.

@oberstet
Copy link
Contributor

great! merging this, since

"autobahn/wamp/serializer.py:314:135: E225 missing whitespace around operator"

is unrelated (flake8 itself changed a bit ..), and is fixed on the upcoming #1382

@oberstet oberstet merged commit 80fe02d into crossbario:master May 12, 2020
@pramodhkp
Copy link
Contributor Author

thank you!

@pramodhkp
Copy link
Contributor Author

@oberstet hey, one question. when is the release for this PR scheduled for?

@pramodhkp
Copy link
Contributor Author

@oberstet could you please give me an ETA for this, if possible? ^

@oberstet
Copy link
Contributor

v20.5.1 should be ready to shi till sunday latest .. and this will have above change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider enabling strict externalPort optionally
2 participants