Last Comment Bug 794180 - mozbuild.compilation.warnings.CompilerWarning should implement rich comparison operators
: mozbuild.compilation.warnings.CompilerWarning should implement rich compariso...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
P5 minor (vote)
: mozilla20
Assigned To: Benedict Singer
:
: Gregory Szorc [:gps] (away until 2017-03-20)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-25 11:24 PDT by Gregory Szorc [:gps] (away until 2017-03-20)
Modified: 2012-12-01 07:39 PST (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rich comparison implementation. (4.43 KB, patch)
2012-11-30 11:40 PST, Benedict Singer
gps: feedback+
Details | Diff | Splinter Review
Updated patch. (4.49 KB, patch)
2012-11-30 12:38 PST, Benedict Singer
gps: review+
Details | Diff | Splinter Review

Description User image Gregory Szorc [:gps] (away until 2017-03-20) 2012-09-25 11:24:05 PDT
From bug 780329:

(In reply to Benjamin Peterson [:benjamin] from comment #55)
> Comment on attachment 651963 [details] [diff] [review]
> Part 7: Define CompilerWarning.__cmp__, v2
> 
> Review of attachment 651963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you want to ensure Python 3 compatibility, you should define rich
> comparison methods instead of __cmp__.

Basically, CompilerWarning sorting won't work in Python 3. Since we only officially support Python 2.7 now, this isn't a huge deal. But, it's nice to have.

I'm putting this up as a mentored bug. The file in question is python/mozbuild/mozbuild/compilation/warnings.py in mozilla-central. You'll need to implement rich comparison operators (__gt__, __ge__, etc) instead of __cmp__. Code will still need to work with Python 2.7. We'll also want a unit test to ensure sorting actually works.
Comment 1 User image Benedict Singer 2012-11-30 11:40:31 PST
Created attachment 687212 [details] [diff] [review]
Rich comparison implementation.

Removes __cmp__ implementation in favor of the rich comparison operators; includes a new unit test for all the comparisons.
Comment 2 User image :Benjamin Peterson 2012-11-30 11:56:57 PST
Comment on attachment 687212 [details] [diff] [review]
Rich comparison implementation.

Review of attachment 687212 [details] [diff] [review]:
-----------------------------------------------------------------

Nice patch. I don't actually understand why CompilerWarning needs to implement anything more than equality and inequality. What is the meaning of one compiler warning being less than the other?

::: python/mozbuild/mozbuild/compilation/warnings.py
@@ -42,5 @@
>      """, re.X)
>  
>  IN_FILE_INCLUDED_FROM = 'In file included from '
>  
> -

This change looks superfluous.

@@ +53,5 @@
>          self['line'] = None
>          self['column'] = None
>          self['message'] = None
>          self['flag'] = None
> +    

You added trailing whitespace here.

@@ +57,5 @@
> +    
> +    # since we inherit from dict, functools.total_ordering gets confused
> +    # thus, we define a key function, a generic comparison, and then
> +    # implement all the rich operators with those; approach is from:
> +    # http://regebro.wordpress.com/2010/12/13/python-implementing-rich-comparison-the-correct-way/

Could you start sentences with capital letters and end them with periods? It's trivial, but it makes the nice comment easier to read.
Comment 3 User image Gregory Szorc [:gps] (away until 2017-03-20) 2012-11-30 11:58:09 PST
Comment on attachment 687212 [details] [diff] [review]
Rich comparison implementation.

Review of attachment 687212 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Just a few minor nits before checkin.

You may also want to see https://developer.mozilla.org/en-US/docs/Creating_a_patch for how to format a proper patch. I'm specifically looking for the author "header" line so this patch is properly attributed to you. I can always grab it from your Bugzilla account info if it's too much trouble for you. But, if you will contribute multiple patches, you'll need to figure this out sooner or later.

::: python/mozbuild/mozbuild/compilation/warnings.py
@@ +41,5 @@
>      (?P<message>.*)
>      """, re.X)
>  
>  IN_FILE_INCLUDED_FROM = 'In file included from '
>  

Nit: This extra space satisfies style guidelines of 2 empty spaces before classes.

@@ +53,5 @@
>          self['line'] = None
>          self['column'] = None
>          self['message'] = None
>          self['flag'] = None
> +    

Nit: Trailing whitespace.

::: python/mozbuild/mozbuild/test/compilation/test_warnings.py
@@ +88,5 @@
> +        w2['line'] = 5
> +        w2['column'] = 5
> +
> +        self.assertLess(w1, w2)
> +        self.assertGreaterEqual(w2, w1)

self.assertGreater

@@ +95,5 @@
> +        w2['line'] = 4
> +        w2['column'] = 6
> +
> +        self.assertLess(w2, w1)
> +        self.assertGreaterEqual(w1, w2)

ditto

@@ +102,5 @@
> +        w2['line'] = 5
> +        w2['column'] = 10
> +
> +        self.assertLess(w1, w2)
> +        self.assertGreaterEqual(w2, w1)

You know what I'm going to say :)
Comment 4 User image Benedict Singer 2012-11-30 12:38:22 PST
Created attachment 687231 [details] [diff] [review]
Updated patch.

Updated with changes from comments above.
Comment 5 User image Gregory Szorc [:gps] (away until 2017-03-20) 2012-11-30 13:08:14 PST
Comment on attachment 687231 [details] [diff] [review]
Updated patch.

Review of attachment 687231 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good aside from the whitespace nit. Also, "rich" in the comment line should be capitalized.

I'll clean these up and commit this patch for you. Congratulation on your first patch!

::: python/mozbuild/mozbuild/compilation/warnings.py
@@ +54,5 @@
>          self['line'] = None
>          self['column'] = None
>          self['message'] = None
>          self['flag'] = None
> + 

Nit: trailing whitespace
Comment 6 User image Benedict Singer 2012-11-30 13:12:29 PST
I left that whitespace in to keep a blank line between the end of that method and the comment before the next method; is that wrong?

Otherwise, thanks!
Comment 7 User image Gregory Szorc [:gps] (away until 2017-03-20) 2012-11-30 13:27:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b997e5ca5ee

The problem was there was a line with just spaces on it. Tidy source code never has trailing white space.

Anyway, this is now checked into the integration tree. It should be merged into the main source tree (mozilla-central) within a few days. When it is, someone will resolve this bug.

Thanks again for contributing! I hope it's not your last :)
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2012-12-01 07:39:58 PST
https://hg.mozilla.org/mozilla-central/rev/6b997e5ca5ee

Note You need to log in before you can comment on or make changes to this bug.