The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla20

Status

()

Core
Build Config
P5
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gps, Assigned: Benedict Singer)

Tracking

Trunk
mozilla20
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
Attachment #687212 - Flags: review?(gps)

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
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 :)
Attachment #687212 - Flags: review?(gps) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 687231 [details] [diff] [review]
Updated patch.

Updated with changes from comments above.
Attachment #687212 - Attachment is obsolete: true
Attachment #687231 - Flags: review?(gps)
(Reporter)

Comment 5

4 years ago
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
Attachment #687231 - Flags: review?(gps) → review+
(Assignee)

Comment 6

4 years ago
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!
(Reporter)

Comment 7

4 years ago
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 :)
Assignee: nobody → singerb.dev
Status: NEW → ASSIGNED
Whiteboard: [mentor=gps][lang=python]
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/6b997e5ca5ee
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.