WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
37850
check-webkit-style: Add __eq__ and __ne__ methods to DefaultStyleErrorHandler
https://bugs.webkit.org/show_bug.cgi?id=37850
Summary
check-webkit-style: Add __eq__ and __ne__ methods to DefaultStyleErrorHandler
Chris Jerdonek
Reported
2010-04-20 04:38:07 PDT
This will facilitate unit testing for
bug 37066
.
Attachments
Proposed patch
(6.79 KB, patch)
2010-04-20 04:51 PDT
,
Chris Jerdonek
hamaji
: review+
hamaji
: commit-queue-
Details
Formatted Diff
Diff
Proposed patch 2
(6.80 KB, patch)
2010-04-20 05:03 PDT
,
Chris Jerdonek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Chris Jerdonek
Comment 1
2010-04-20 04:51:59 PDT
Created
attachment 53796
[details]
Proposed patch
Chris Jerdonek
Comment 2
2010-04-20 05:03:00 PDT
Created
attachment 53798
[details]
Proposed patch 2 Tweaked two docstrings (added missing "the").
Shinichiro Hamaji
Comment 3
2010-04-20 05:10:31 PDT
Comment on
attachment 53796
[details]
Proposed patch Looks good.
> + def test_eq__true_return_value(self):
Maybe too many underscores after "eq"?
> + def test_eq__false_return_value(self):
Maybe too many underscores after "eq"?
Chris Jerdonek
Comment 4
2010-04-20 05:27:27 PDT
(In reply to
comment #3
)
> (From update of
attachment 53796
[details]
) > Looks good. > > > + def test_eq__true_return_value(self): > > Maybe too many underscores after "eq"? > > > + def test_eq__false_return_value(self): > > Maybe too many underscores after "eq"?
This is a style I learned from working on the Python source code and that I think is nice. The style is to give unit tests names of the form-- test_methodbeingtested__usecasedescription The double underscore is helpful because "methodbeingtested" itself often has underscores. You can see an example of this style in the unit tests for the unittest module itself:
http://svn.python.org/view/python/trunk/Lib/unittest/test/test_loader.py?revision=79464&view=markup
Do you think this style is okay? Note that this is a somewhat unusual case to start using the style with since __eq__ itself begins and ends with two underscores. But it is probably best to leave those out of "methodbeingtested" in this case, or else we would have four consecutive underscores in the name. :)
Chris Jerdonek
Comment 5
2010-04-20 05:35:28 PDT
If you'd like, for consistency I could adjust the names of the other test methods in the class. They would like something like-- test_call__non_reportable_error instead of-- test_non_reportable_error By the way, thanks for reviewing so quickly!
Shinichiro Hamaji
Comment 6
2010-04-20 05:44:18 PDT
Comment on
attachment 53798
[details]
Proposed patch 2 I see. Thanks for your explanation. I have no preference for this. Please name methods as you like. I guess it's worth writing this rule in our python guideline?
Chris Jerdonek
Comment 7
2010-04-20 05:59:12 PDT
(In reply to
comment #6
)
> (From update of
attachment 53798
[details]
) > I see. Thanks for your explanation. I have no preference for this. Please name > methods as you like. I guess it's worth writing this rule in our python > guideline?
Good suggestion. I added a note about this on the wiki page:
https://trac.webkit.org/wiki/PythonGuidelines#Style
WebKit Commit Bot
Comment 8
2010-04-20 12:34:17 PDT
Comment on
attachment 53798
[details]
Proposed patch 2 Clearing flags on attachment: 53798 Committed
r57905
: <
http://trac.webkit.org/changeset/57905
>
WebKit Commit Bot
Comment 9
2010-04-20 12:34:22 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 10
2010-04-20 13:54:38 PDT
http://trac.webkit.org/changeset/57905
might have broken Windows Debug (Tests) The following changes are on the blame list:
http://trac.webkit.org/changeset/57904
http://trac.webkit.org/changeset/57905
http://trac.webkit.org/changeset/57906
http://trac.webkit.org/changeset/57907
http://trac.webkit.org/changeset/57908
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug