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

[9413] Conch documentation fixes #991

Merged
merged 7 commits into from
Jul 7, 2018
Merged

Conversation

wiml
Copy link
Contributor

@wiml wiml commented Apr 6, 2018

@codecov
Copy link

codecov bot commented Apr 7, 2018

Codecov Report

Merging #991 into trunk will decrease coverage by 0.35%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            trunk     #991      +/-   ##
==========================================
- Coverage   91.84%   91.49%   -0.36%     
==========================================
  Files         844      842       -2     
  Lines      150593   149575    -1018     
  Branches    13141    13089      -52     
==========================================
- Hits       138318   136858    -1460     
- Misses      10178    10605     +427     
- Partials     2097     2112      +15

@wiml
Copy link
Contributor Author

wiml commented Apr 8, 2018

This PR doesn't change any code (only docstrings), so I think the codecov failure is spurious.

Copy link
Member

@markrwilliams markrwilliams left a comment

Choose a reason for hiding this comment

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

Thanks for improving conch's documentation!

I agree that the Codecov failure is spurious. I've pushed this to a branch so the builders will run on it and this can pass all required checks.

Feel free to address the review comments asking when methods can return None in a follow up PR.

Thanks again!


@type channelType: L{str}
@param channelType: The requested channel type
@type channelType: L{bytes}
Copy link
Member

Choose a reason for hiding this comment

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

Whoops! This certainly appears to be bytes. Thanks for fixing the type here!

@param subsystem: The name of the subsystem being requested
@type subsystem: L{bytes}
@param data: Additional request data (often nothing)
@type data: L{bytes}
Copy link
Member

Choose a reason for hiding this comment

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

👍 This is patently binary data.

@type data: L{str}
@rtype: subclass of L{SSHChannel}/L{tuple}
@param data: Additional request data
@type data: L{bytes}
Copy link
Member

Choose a reason for hiding this comment

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

👍 Also binary data (from ssh_CHANNEL_OPEN).


We return a L{Protocol}.
@param subsystem: The name of the subsystem being requested
@type subsystem: L{bytes}
Copy link
Member

Choose a reason for hiding this comment

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

👍 Binary data!

@type subsystem: L{bytes}
@param data: Additional request data (often nothing)
@type data: L{bytes}
@rtype: L{Protocol} or L{None}
Copy link
Member

Choose a reason for hiding this comment

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

When can this be None?

@rtype: subclass of L{SSHChannel}/L{tuple}
@param data: Additional request data
@type data: L{bytes}
@rtype: a subclass of L{SSHChannel} or L{None}
Copy link
Member

Choose a reason for hiding this comment

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

When can this be None?

@wiml
Copy link
Contributor Author

wiml commented Apr 8, 2018

I added a few sentences to explain the None return values.

@twm
Copy link
Contributor

twm commented Jul 7, 2018

The Fedora buildbot is failing with nonsense. o.O

[ERROR]
Traceback (most recent call last):
  File "/buildslave/fedora26-python-3.6/Twisted/build/py36-alldeps-nocov-posix/lib/python3.6/site-packages/twisted/trial/_synctest.py", line 1375, in _runCleanups
    f(*args, **kwargs)
  File "/buildslave/fedora26-python-3.6/Twisted/build/py36-alldeps-nocov-posix/lib/python3.6/site-packages/twisted/internet/test/reactormixins.py", line 225, in unbuildReactor
    reactor._uninstallHandler()
  File "/buildslave/fedora26-python-3.6/Twisted/build/py36-alldeps-nocov-posix/lib/python3.6/site-packages/twisted/internet/posixbase.py", line 324, in _uninstallHandler
    self._childWaker.uninstall()
  File "/buildslave/fedora26-python-3.6/Twisted/build/py36-alldeps-nocov-posix/lib/python3.6/site-packages/twisted/internet/posixbase.py", line 214, in uninstall
    _signals.installHandler(-1)
  File "/buildslave/fedora26-python-3.6/Twisted/build/py36-alldeps-nocov-posix/lib/python3.6/site-packages/twisted/internet/_signals.py", line 54, in installHandler
    signal.signal(signal.SIGCHLD, signal.SIG_DFL)
  File "/usr/lib64/python3.6/signal.py", line 47, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
builtins.TypeError: 'int' object is not callable

twisted.internet.test.test_process.ProcessTestsBuilder_EPollReactorTests.test_systemCallUninterruptedByChildExit

Clearly nothing to do with this change.

@twm twm merged commit 73cfe5e into twisted:trunk Jul 7, 2018
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.

None yet

3 participants