pollingfile._PollableWritePipe should check for unicode earlier

[zseil]

pollingfile._PollableWritePipe currently checks for outgoing unicode data in checkWork, its equivalent of FileDescriptor.doWrite. This is too late, the user's code will not be notified of this error, it will just break the _PollingTimer to which the pipe is attached. The check for unicode data should be made in write and writeSequence instead.

[moijes12]

The check for unicode has been removed from _PollableWrite.checkWork. Checks for unicode have been added to _PollableWritePipe.write and _PollableWritePipe.writeSequence. The unit tests have been added to test_pollingfile.TestPollableWritePipe and the test involving checkWork has been removed. Do note that I've created this patch using Eclipse as I'm trying to setup a work environment and if this causes any issues, please let me know. I'll make the required changes.

[ivank]

Thanks for the patch!

1. Needs a NEWS file. See http://twistedmatrix.com/trac/wiki/ReviewProcess

2. Because writeSequence accepts any sequence, including sequences that can only be iterated once (like a generator), you must not iterate over the sequence twice. (First, map iterates, then extend iterates again. The sequence could be empty the second time.)

3. Please add a test for the above, so this doesn't get broken in the future.

4. I think there's an extra colon here: @type seq: : A C{sequence}

5. Please remove the trailing whitespace you added to many locations. Also, blank lines should have no whitespace/indent.

6. Nit: please end your docstring sentences with a period.

Thanks again. I couldn't do a full review, so please put it back into review after you fix those things.

Also, I am uncertain as to what arguments write and writeSequence accept (does it take buffer objects too?). Perhaps the next reviewer could take a look.

[exarkun]

2. Because writeSequence accepts any sequence, including sequences that can only be iterated once (like a generator), you must not iterate over the sequence twice. (First, map iterates, then extend iterates again. The sequence could be empty the second time.)

This might be a nice thing to eventually handle, but twisted/internet/abstract.py iterates twice, so I think in practice we don't support these kinds of sequences.

Also, I am uncertain as to what arguments write and writeSequence accept (does it take buffer objects too?). Perhaps the next reviewer could take a look.

twisted/internet/abstract.py probably works with buffers, but I don't know if we have any test coverage for this. It would probably be good if it worked with buffer, but adding tests for that and making any necessary changes to get them to pass could be a separate ticket.

[moijes12]

I've attached the patch. Its nearly identical to the previous patch except for the newsfile. Thats because going by the exarkun's comments, twisted doesn't support sequences which can be iterated over only once. I've added the tests and made the required formatting changes. I was unable to test this as I was unable to build twisted on a windows PC and the tests were skipped in Linux. I hope this is good enough.

[exarkun]

(In [34061]) Branching to 'pollingfile-write-unicode-5412'

[exarkun]

(In [34062]) Apply 5412.patch

refs #5412

[itamar]

Thanks for the updated patch!

1. C{sequence} suggests there's some sort of Python object known as "sequence", which there isn't; the @type is therefore both wrong and unhelpful. Perhaps iterable or just plain sequence would be better, and omit the @type in writeSequence.
2. There's a dangling period on a line by itself on line 256 of _pollingfile.py.
3. You should have two lines of whitespace between methods; this is old code that predates that, probably, but since you're modifying it you should add least fix that for the methods you are touching.
4. You probably want to say C{seq} on line 245 of _pollingfile (the @raise).
5. Docstrings for write and writeSequence should talk about something like "Write bytes" and "Write a list or other sequences of bytes"; referring to "data" is too vague.
6. The news file could omit all but the first of the references to twisted.intenet._pollingfile._PollableWritePipe, for brevity's sake ("checks for outgoing unicode in .write()" etc.)

You can run tests yourself using force-builds.py (see  http://labs.twistedmatrix.com/2011/09/pre-trunk-testing.html), but unfortunately only if you have a branch; when you submit the final patch I will run them for you.

[moijes12]

Going by the comments by itamar

1. Points 1,2,3 and 6 done as suggested.
2. Point 4 - No. I meant the input seq. I don't know what a C{seq} is. Please help.
3. Point 5 - I've made the changes. But I'm afraid they'll still need more fine-tuning. Let me know what you think of them.

[itamar]

BTW, if you're providing patches and a branch already exists, please provide a patch against the branch.

"C{seq}" is epytext markup. It means the API documentation output will use a code styling for the output, which is useful in emphasizing that this you're referring to an input variable.

In any case, I've applied the patch, tweaked the docstring, and will do final pass if the tests pass:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/pollingfile-write-unicode-5412

[itamar]

Looks good, will merge as soon as I see a green Windows buildslave.

[itamarst]

(In [34212]) Merge pollingfile-write-unicode-5412: Check for unicode writes earlier in _pollingfile.

Author: moijes12 Review: ivank, exarkun, itamar Fixes: #5412

Check for unicode writes earlier in pollingfile._PollableWritePipe, so it's clear which bit of user's code messed up.