divmod-dev team mailing list archive

[Jean-Paul Calderone <exarkun@xxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks.  It will be interesting to see what kind of custom reporters people build for pyflakes once this lands.

  0. Perhaps the reporter shouldn't be defined in the scripts module.
  1. These three methods of the reporter, ioError, problemDecodingSource, and syntaxError seems bad to me.  These are all just reasons pyflakes couldn't get an AST.  They're all quite well described by some exception object that pyflakes catches.  And it's entirely possible we'll find other problems that fall into the same category.  It would be better to have a single method that dealt with these errors and could be expanded to handle new errors rather than having a method for each and have to add a new method to the reporter interface.
  2. Can we consider accepting (and explicitly documenting) text streams to the reporter instead of byte streams?  All of the output pyflakes generates should be text.
  3. Seems like there is an excessive use of `os.path` for a project that already depends on Twisted and even uses FilePath in a few places.
  4. Some things are missing docstrings: test_flake, CheckTests.getErrors, 
  5. Thanks for the makeTempFile / assertHasErrors refactoring.  That's nice.
  6. popen is deadly poison.  I think it would be really great if pyflakes tests didn't need to use it, somehow.  At a absolute minimum, it would be nice if none of the details of its use leaked out of the single helper method for using it.  eg, no use of `communicate` in individual test methods.
-- 
https://code.launchpad.net/~jml/divmod.org/pyflakes-reporter/+merge/113857
Your team Divmod-dev is subscribed to branch lp:divmod.org.