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-
DetailsFormatted DiffDiff
Proposed patch 2 (6.80 KB, patch)
2010-04-20 05:03 PDT, Chris Jerdonek 		no flags
DetailsFormatted DiffDiff
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.
Note You need to log in before you can comment on or make changes to this bug.