[webkit-reviews] review denied: [Bug 65823] Fix SCM webkitpy unit test failures : [Attachment 103218] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 8 09:11:34 PDT 2011
Eric Seidel <eric at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 65823: Fix SCM webkitpy unit test failures
https://bugs.webkit.org/show_bug.cgi?id=65823
Attachment 103218: Patch
https://bugs.webkit.org/attachment.cgi?id=103218&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103218&action=review
Aside from the timezone thing, this change looks great!
> Tools/Scripts/webkitpy/common/checkout/scm/git.py:206
> + if not os.path.exists(path):
This should be self.filesystem.exists (assuming SCM/Git has a filesystem
object?)
>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:538
>> + has_tz = os.environ.has_key('TZ')
>
> .has_key() is deprecated, use 'in' [pep8/W601] [5]
I think you mean 'TZ' in os.environ?
> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:548
> + old_tz = os.environ.get('TZ', '')
> + os.environ['TZ'] = 'PST8PDT'
> + time.tzset()
> + changelog_entry = changelog_entry.replace('DATE_HERE',
date.today().isoformat())
> + if hasattr(time, 'tzset'):
> + if has_tz:
> + os.environ['TZ'] = old_tz
> + else:
> + del os.environ['TZ']
> + time.tzset()
Really? This can't be right. We can't override the timezone in some cleaner
way?
> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:614
> +""" % {'whitespace': ' '}
This is to get around the tools wanting to remove trailing whitespace?
More information about the webkit-reviews
mailing list