Bug 37850 - check-webkit-style: Add __eq__ and __ne__ methods to DefaultStyleErrorHandler
Summary: check-webkit-style: Add __eq__ and __ne__ methods to DefaultStyleErrorHandler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks: 37066
  Show dependency tree / graph
 
Reported: 2010-04-20 04:38 PDT by Chris Jerdonek
Modified: 2010-04-20 13:54 PDT (History)
6 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-04-20 04:38:07 PDT 
This will facilitate unit testing for bug 37066.
Comment 1 Chris Jerdonek 2010-04-20 04:51:59 PDT 
Created attachment 53796 [details]
Proposed patch
Comment 2 Chris Jerdonek 2010-04-20 05:03:00 PDT 
Created attachment 53798 [details]
Proposed patch 2

Tweaked two docstrings (added missing "the").
Comment 3 Shinichiro Hamaji 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"?
Comment 4 Chris Jerdonek 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. :)
Comment 5 Chris Jerdonek 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!
Comment 6 Shinichiro Hamaji 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?
Comment 7 Chris Jerdonek 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
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2010-04-20 12:34:22 PDT 
All reviewed patches have been landed.  Closing bug.