divmod-dev team mailing list archive

[Jean-Paul Calderone <exarkun@xxxxxxxxxxxxxxxxx>]

Review: Needs Information

> Reporter moved to new module, pyflakes.reporter.

Great, thanks.

> Not sure these can be combined into one method while still preserving current output. 

Okay, that makes sense.  I guess pyflakes doesn't offer any API compatibility promises anyway, so we can change this as needed later. ;)

> Done, but for tests only. AFAICT, only the tests import Twisted. Happy to change non-test code if you don't mind having it depend on Twisted.

Just making the tests nice is fine for now, as far as I'm concerned.

> Docstrings added

Great.

> I've killed subprocess.Popen usage. Made an ugly compromise for spawning a subprocess and sending stdin. Think it's the least of four evils (the other three being continue w/ subprocess.Popen, duplicate _EverythingGetter in pyflakes, or block this branch until Twisted provides such a helper).

Hmm.  This is going to break hard at some point, though.  Though, it will only break the unit tests, not pyflakes itself...  Still, `_EverythingGetter` is barely any longer than the subclass being added to pyflakes.  Duplicating it in pyflakes seems better than waiting for pyflakes to break when `twisted.internet.utils` changes.  Also, `_EverythingGetter` is awful (for example, errbacking with a tuple if a signal ends the process).  I'd lean towards copying it (and simplifying it by fixing it to get rid of the nasty signal behavior, at least) instead of importing the private name.  If you're not convinced, fine (but I might ask you to fix it when Twisted breaks it ;).

Thanks!  Do you plan to do point 2 (text streams) or do you want to delay that for a separate ticket (is there even a ticket associated with this merge proposal)?
-- 
https://code.launchpad.net/~jml/divmod.org/pyflakes-reporter/+merge/113857
Your team Divmod-dev is subscribed to branch lp:divmod.org.