pekka.klarck commented on revision r1550 in project robotframework.
Details are at http://code.google.com/p/robotframework/source/detail?r=1550
Score: Positive
General Comment:
Good fix for stupid bugs. I especially like the new Processor class (which
could actually be renamed to Tidyer (or is it Tidier??)). Please take a
look at the line-by-line comments, though, since there are some bugs and
few more places for cleanup in this implementation.
Line-by-line comments:
File: /trunk/tools/robotidy/robotidy.py (r1550)
===============================================================================
Line 97: from robot.output import SYSLOG
-------------------------------------------------------------------------------
Must also call SYSLOG.register_command_line_monitor at some point to make
parsing errors and warnings visible. There should be plenty of
errors/warnings e.g. when you run ./testidy.sh.
Line 122: utils.NormalizedDict.__contains__ = utils.NormalizedDict.has_key
-------------------------------------------------------------------------------
Remove this also when editing. This version won't work even with 2.0 due
to SYSLOG changes so pre 1.8.3 compatibility isn't really needed.
Line 190: self.errors = []
-------------------------------------------------------------------------------
Errors should be logged using SYSLOG instead of storing them and printing
at the end.
Line 198: outfile = infile
-------------------------------------------------------------------------------
Move this if/else outside try/except because it cannot fail.
Line 205: raise
-------------------------------------------------------------------------------
utils.get_error_message below takes care of re-raising KeyboardInterrupt
(and also SystemExit which should be listed here otherwise).
Line 209: self.errors.append(error_msg)
-------------------------------------------------------------------------------
It seems to me that error is first printed here but also added to
self.errors and printed again at the end. As I already commented,
self.errors can be removed.
Actually also print should be removed and replaced with SYSLOG.error.
Line 227: if tail.split('.')[-1] in ['tsv', 'xhtml', 'html']:
-------------------------------------------------------------------------------
1) Would be much better to use global _valid_extensions attribute in the
beginning of this module here than listing extensions again. Especially
when 'htm' is missing from this list.
2) The canonical way to split extensions is using os.path.splitext. Here
split('.') is pretty safe, but there's a minor bug that files with
name 'tsv' or 'html' without extension at all would be processed
('tsv'.split('.') == 'tsv'). Should thus switch to os.path.splitext here
too.
Line 725: pass
-------------------------------------------------------------------------------
This KeyboardInterrupt can be removed too.
Line 727: SYSLOG.error(utils.get_error_message())
-------------------------------------------------------------------------------
This could be changed just to
except DataError, err:
SYSLOG.error(str(err))
Line 730: if len(processor.errors) > 1:
-------------------------------------------------------------------------------
This should be "if processor.errors" but as commented above
Processor.errors should be removed altogether and this if block obviously
too.
Respond to these comments at
http://code.google.com/p/robotframework/source/detail?r=1550
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
