794180 - mozbuild.compilation.warnings.CompilerWarning should implement rich comparison operators

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P5)

Description

[Gregory Szorc [:gps]]

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]

Attachment: 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]

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]]

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]

Attachment: Updated patch.

Updated with changes from comments above.

Comment 5

[Gregory Szorc [:gps]]

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]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/6b997e5ca5ee