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

Fix connect_srv()'s arguments to include bind_port #759

Closed
wants to merge 1 commit into from

Conversation

mijofa
Copy link

@mijofa mijofa commented Oct 23, 2023

Fixes this ~3yr old bug #493

I think it worth considering reworking this function to use *args, **kwargs for these extra arguments, or at least call connect() with keyword arguments.
Either one would reduce issues with future failings to manually keep the functions in sync.
But that will likely raise a more intensive workflow discussion, and my goal for this is just to get this 3yr old bug fixed so I can stop using my terrible workarounds.

@petersilva
Copy link
Contributor

This adds an argument in the middle of an existing entry_point, so anyone using positional parameters will see breakage. Is there a good readon for that? Can/should we avoid breakage by putting the new parameter at the end?

@petersilva
Copy link
Contributor

You need to convince github that you have signed the ECA.

@mijofa
Copy link
Author

mijofa commented Oct 24, 2023

This adds an argument in the middle of an existing entry_point, so anyone using positional parameters will see breakage. Is there a good readon for that? Can/should we avoid breakage by putting the new parameter at the end?

It is currently completely broken, making this function 100% unusable,
this has been the case for at least 3yrs. (ref: issue #493)

I figured since it's already completely broken then might as well stick to consistency with the underlying connect() function.
This, however, is what my suggestion of using *args, **kwargs for the arguments being passed through directly is meant to kind of resolve and future-proof.
Calling connect() with keyword arguments would also avoid future issues with arguments changing,
but that would not allow connect_srv() to make use of any arguments added to connect() in future.

You need to convince github that you have signed the ECA.

I don't have an Eclipse account, nor do I want one, I just want this 3yr old bug fixed.
I've pushed a 5min patch and suggested some further improvements,
and now want to wash my hands of this and move on.

I've read the ECA and can certify that:

The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file;

and

I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my signoff) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@PierreF
Copy link
Contributor

PierreF commented Jan 7, 2024

Thank for your contribution, but I'm not sure a simple comment that you agree on the ECA is enough for Eclipse process.

While I added typing, I've hit this same issue and fixed it in #791.

I'm closing this PR in favor of #791

@PierreF PierreF closed this Jan 7, 2024
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.

3 participants