Jump to conversation
Unresolved conversations (13)
@glyph glyph Jan 31, 2017
Minor nitpick: I prefer the style where this parenthesis is un-indented, so it's at the same indent level as the line with the open paren on it.
Outdated
src/twisted/internet/unix.py
exvito
@glyph glyph Jan 31, 2017
This could be a little clearer. Don't get me wrong, it's very precise! Technically, you're adhering to the letter of the law for news fragments, by calling out `twisted.internet.unix.Server.doRead` by its full name, but `doRead` is probably not the API of interest to users here; UNIX endpoints (i.e. [`serverFromString("unix:...")`](https://twistedmatrix.com/documents/16.6.0/api/twisted.internet.endpoints.html#serverFromString) or `UNIXServerEndpoint` are the things that are going to be invoked. Generally, as you're writing NEWS fragments, imagine you're a user of Twisted interested in this specific feature, and you're trying to understand what has changed. Reactor internals often have to change to support some other thing, so focus on the other thing. Also: what's a "supported" level or type?
Outdated
src/twisted/topfiles/8912.bugfix
exvito
@glyph glyph Jan 31, 2017
Ideally this newline should be removed; it's unnecessary, per the coding standard.
Outdated
src/twisted/internet/unix.py
exvito
@exarkun exarkun Jan 18, 2017
(optional) The need to monkey-patch here suggests the implementation should probably accept `recvmsg` (and so probably also `sendmsg`) as parameters - thus removing the need for monkey-patching. Making the sendmsg module a class attribute of `_SendmsgMixin` would probably accomplish this well-enough. That would allow the fake sendmsg to just be set on the `FakeReceiver` instance.
Outdated
src/twisted/internet/test/test_unix.py
exvito
@exarkun exarkun Jan 18, 2017
(required) This is unsupported now but `SCM_RIGHTS+1` is `SCM_CREDENTIALS`. `SOL_SOCKET+1` doesn't appear to collide but I may be missing something and some other platform may have different constants. It looks like these are int fields. There's probably room to go far, far, far away from the currently assigned values? Or, alternatively, to pick actual values that are known to be invalid in this context (with a comment). For example ... `SOL_TCP`? `SOL_UDP`? Unfortunately Python doesn't expose many `SCM_*` constants (any, on Python 2...) making this strategy difficult there. Ah, of course this is all happening at the Python level. So an out-of-bounds-for-`int` value would probably also work to guarantee non-support in the implementation.
Outdated
src/twisted/internet/test/test_unix.py
exvito
@exarkun exarkun Jan 18, 2017
(required) The monkey-patching should be done using `self.patch(sendmsg, "recvmsg", fakeRecvmsgUnsupportedAncillary)`.
Outdated
src/twisted/internet/test/test_unix.py
exvito
@exarkun exarkun Jan 18, 2017
(required) Hmm. The existing assertion doesn't verify that the order of file descriptors received is the same as the order of file descriptors sent? So if the implementation mixed them up we wouldn't notice... Is that right? If so, that seems moderately unfortunate. It's a pre-existing defect so it could be fixed in a follow-up ticket. Please either fix it here or file that follow-up ticket.
src/twisted/internet/test/test_unix.py
exvito
@exarkun exarkun Jan 18, 2017
(optional) Given the structure of this code, I'm excited to introduce support for receiving other kinds of ancillary data. It looks like it should be quite straightforward to do so. If you wanted, you could take one additional step towards this and improve the way dispatch is done here. For example, the fd handling logic could move into an `_ancillary_SOL_SOCKET_SCM_RIGHTS` method which is invoked via dict-style lookup. The fall-through case would then be the log-and-drop logic. However, for the single case that's actually supported now, the code is perfectly fine as-is. Also, I know there's also `SCM_CREDENTIALS` (and googling turns up some really obscure stuff like `SCM_WIFI_STATUS`). I don't know if there's anything else useful beyond these two ... If there are only two then dict-based dispatch is not _much_ of a win (certainly not performance; perhaps still readability).
Outdated
src/twisted/internet/unix.py
exvito
@exarkun exarkun Jan 18, 2017
(optional) It seems like it should be the case that new code should use `twisted.logger` instead of `twisted.python.log`. I don't know if this is really true. It would probably be a good thing to document somewhere. That said, this module already uses `twisted.python.log` everywhere so this message fits in with the existing code fairly well. Unless there's an explicit policy to the contrary, I don't have a problem, with leaving this as-is and doing the logging conversion work at some later point.
Outdated
src/twisted/internet/unix.py
exvito
@glyph glyph Jan 14, 2017
This is an instance of http://xunitpatterns.com/Conditional%20Test%20Logic.html. Skips should be computed outside of the tests. The failure mode of computing a skip in-line like this is that a _buggy implementation_ looks the same to us as an _unsupported platform_. Explicitly check if it's a known-to-not-support-multiple-sendmsg()s platform, then skip the test by setting its `skip` attribute on the outside.
Outdated
src/twisted/internet/test/test_unix.py
exvito
@glyph glyph Jan 14, 2017
These imports should really be moved up to the top level. I realize you're just copying the surrounding pattern, but this is making a bad pattern worse :).
Outdated
src/twisted/internet/test/test_unix.py
exvito
@glyph glyph Jan 14, 2017
There needs to be an `@param` here describing `ancillaryPacker`.
Outdated
src/twisted/internet/test/test_unix.py
@glyph glyph Jan 14, 2017
This S should be lower-case to adhere to the coding standard.
Outdated
src/twisted/internet/test/test_unix.py
Resolved conversations (0)