Hi Kristina,
That's a lot of code but fortunately just a few regular patterns. It
looks good. I just have a couple of comments:
Harold
common.py:
388: I don't believe that try/except should be used as a control
structure. It seems more correct and natural to write this as:
if log_level in self._log_lvl_convert_to_method:
self._log_lvl_convert_to_method[log_level]()
else:
raise ValueError
test_js2ai.py:
604,663: Should these lines be commented out?
On 08/ 3/11 09:44 AM, Kristina Tripp wrote:
If someone has some time I could still use a review of this.
Thanks
- Kristina
On Jul 29, 2011, at 1:09 PM, Kristina Tripp wrote:
Please find the following for review
7069014 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7069014>
log error messages don't identify type of error
https://cr.opensolaris.org/action/browse/caiman/enpointe/CR_7069014/webrev/
There's allot of changes here but there all basically the same so
it's not as bad as it looks since most of the changes are related to
reducing the complexity of how logging and error reporting was being
handled in js2ai. Basically every log call in the code base was
changed. Except for 2 minor changes in 2 error messages (I don't
recall which ones) no text in the error messages was changed.
The main goal of this checkin was to add the error level to the log
message. This changes the log format from
profile:line 90: can not create c1t0d0s1 with a size of 'all'.
Conflicts with c1t0d0s0 that was created earlier via keyword
'filesys' with a size of 40000mb.
to
profile:line 90:CONVERSION: can not create c1t0d0s1 with a size of
'all'. Conflicts with c1t0d0s0 that was created earlier via keyword
'filesys' with a size of 40000mb.
To accomplish this and continue using the python logger, 4 new
logging levels where added (see __init__.py:252). The formater for
the logger was changed to
"%(file)s:line %(line_num)d:%(levelname)s: %(message)s".
Since the logger doesn't now about "file" or "line_num" it was
necessary to make use of the extra parameter for logging. The extra
parameter is for passing a dictionary to the log routines that the
logger uses to lookup and fill in values to use for these keywords
when the log message is being generated.
A simple example would be
format = _("%(file)s:line %(line_num)d:%(levelname)s: %(message)s")
logging.Formatter(format)
extra_log_params = {"file": "profile1", "line_num": 10 }
logger.warning('error message', extra=extra_log_params)
Which would generate
profile1: line 10: WARNING: error message
Since as part of this work the log level was now being used by the
log message it made sense to create a wrapper for log file generation
that also set the conversion_report error level.
With the changes included in this code review the complexity of the
log and error generation has changed from
LOGGER.error(_("%(file)s: line %(lineno)d: " \
"Incomplete rule detected") % \
{"file": RULES_FILENAME, "lineno": line_num})
conv_report.add_process_error()
to
generate_error(LVL_PROCESS, conv_report,
_("Incomplete rule detected"),
extra_log_params)
Overall it makes the error message in js2ai easier to read.
The use of extra_log_params dictionary negated the need to pass
line_num all around the code base as such it was removed from most
function calls.
Tests where updated to reflect the API changes.
Pep8 and pylint run against code base
*Kristina Tripp, Senior Software Engineer*
OracleRevenue Product Engineering
500 Eldorado Blvd, MS UBRM05-171
Broomfield, CO, 80021
Office: 303-272-8655
Email: [email protected] <mailto:[email protected]>
Oracle is committed to developing practices and products that help
protect the environment
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
