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

for issue #116 #122

Closed
wants to merge 2 commits into from
Closed

for issue #116 #122

wants to merge 2 commits into from

Conversation

0-harshit-0
Copy link

added functionality to connect using URL

@0-harshit-0
for issue #116
d77c911 
added functionality to connect using URL
@atagar
Copy link
Contributor

atagar commented Jun 27, 2022

Hi harshit, sorry for the long delay. This is a fine attempt but has some issues...

  1. It breaks both method's pydocs. The first comment block of a method is special in that Sphinx uses it to generate API documentation.
  2. Rather than allowing hostnames for the address argument it would be better if we added a new 'hostname' argument. This way the argument isn't a misnomer and can't accidently trigger a DNS lookup (which could leak data in some environments).
  3. This change would require unit and integ test coverage.

@atagar atagar mentioned this pull request Jun 27, 2022
Open
@0-harshit-0
Copy link
Author

oh ok got it.

@0-harshit-0
Update control.py
e6d186f 
added hostname argument
@0-harshit-0
Copy link
Author

hello, I have made a new commit to this pull request.

@atagar
Copy link
Contributor

atagar commented Sep 27, 2022

It's been a while without tests being added so closing.

@atagar atagar closed this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@0-harshit-0 @atagar