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
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-25 11:24 PDT by Gregory Szorc [:gps]
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 Gregory Szorc [:gps] 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 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 :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 Gregory Szorc [:gps] 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 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 Gregory Szorc [:gps] 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 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 Gregory Szorc [:gps] 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 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.