pekka.klarck commented on revision r2060 in project robotframework.
Details are at http://code.google.com/p/robotframework/source/detail?r=2060
Score: Negative
General Comment:
The fix is apparently good, but the resulting code is pretty much
impossible to understand without knowing the background. The code must be
refactored. Additionally some print statements are left to the code and
documentation could perhaps be slightly enhanced.
Line-by-line comments:
File: /trunk/src/robot/libraries/BuiltIn.py (r2060)
===============================================================================
Line 538: `Set Suite Variable`.
-------------------------------------------------------------------------------
1) Why this note is added only into this keyword and not to others?
2) I'm not sure is "See also not from ..." correct English. Would "See ...
for more information" be enough?
Line 665: | Set Suite Variable | ${NAME} | new value | # This does
not work |
-------------------------------------------------------------------------------
Good example but have you looked at how all this looks like in HTML
documentation? 'Note:' may not show that nicely there.
Line 696: name = self._get_variable_name_in_uk(origname) or origname
-------------------------------------------------------------------------------
Having 'or' in the end of a long statement makes the whole line really hard
to read. Why doesn't '_get_variable_name_in_uk' return 'origname' itself
instead of 'None'?
These methods should also get better names. They were already bad
(_get_var_name and _get_var_name_helper) but adding new helper with
mysterious name made it even worse. Without knowing why all these helpers
are here it's impossible to know what's happening. This might even need a
comment with a reference to the fixed bug, but naming should still be
enhanced.
Line 700: if not name:
-------------------------------------------------------------------------------
I'm pretty sure these 'print' statements are left here by accident.
Line 713: return name
-------------------------------------------------------------------------------
Actually it might be best to merge this with the current
_get_var_name_helper. The old method even has sanity check that the given
item is a non-empty string which is now ignored because this method blows
up if its not a string.
Respond to these comments at
http://code.google.com/p/robotframework/source/detail?r=2060
--
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
