This is an archive of the discontinued LLVM Phabricator instance.

[LNT] Python 3 support: Update `<>` and `None` comparisons
ClosedPublic

Authored by hubert.reinterpretcast on Sep 12 2019, 9:29 PM.

Details

Summary

Replaces comparisons using !=, <>, and == against None with the corresponding version of is not None and is None.

Replaces <> with !=.

As requested by reviewers, add spaces around operators on lines touched.

Diff Detail

Repository
rL LLVM

Event Timeline

I haven't put a comment for all occurences of the issues I pointed. Please make a scan through the patch for the whitespaces and isinstance suggestions.

lnt/external/stats/pstat.py
133 ↗(On Diff #220041)

I think a more pythonic change would be:

if !isinstance(source, (list, tuple))

Similarly below.

222 ↗(On Diff #220041)

This should be a straight isinstance. While the more complex case with several types are not caught by futurize, that one is changed as expected by futurize. I would recommend you to give it a try.

293 ↗(On Diff #220041)

I'm quite new to Python but I believe for comparison against None it is more usual to do "is None" or "is not None".

lnt/external/stats/stats.py
515 ↗(On Diff #220041)

Please add whitespace around operator while you are at it.

lnt/server/reporting/runs.py
126 ↗(On Diff #220041)

I am not sure it is safe in this specific case to do this. With the old syntax if you passed a tuple with 3 parameters the function would behave as you would expect it (by doing tuple unpacking first) while with the new code I think it'll give an error.

Futurize gave me instead (except for the variable naming):

lambda bucket_entry: -abs(bucket_entry[1].pct_delta)

If you've checked that the function is always called as expected then the change you propose is fine (and better).

lnt/server/ui/views.py
833 ↗(On Diff #220041)

As above.

BTW given the size of this patch it might be worth splitting into smaller bits (eg. the type changes, the lambda one and the <> one).

BTW given the size of this patch it might be worth splitting into smaller bits (eg. the type changes, the lambda one and the <> one).

I'll split out the lambda and raise changes first.
Then it looks like:

  • Updating the type-related comparisons.
  • Changing the remaining <> cases.
  • Changing the remaining type references (if any).
lnt/external/stats/pstat.py
133 ↗(On Diff #220041)

I'll look into updating to use isinstance.

293 ↗(On Diff #220041)

Okay. Will do.

lnt/server/reporting/runs.py
126 ↗(On Diff #220041)

The problem you describe is likely. Will fix.

lnt/server/ui/views.py
833 ↗(On Diff #220041)

Will fix.

cmatthews added inline comments.Sep 13 2019, 10:43 AM
lnt/external/stats/pstat.py
293 ↗(On Diff #220041)

space between each list elements.

hubert.reinterpretcast marked 13 inline comments as done.Sep 14 2019, 10:24 AM

Split out changes for lambdas (D67582), raise (D67581), type-related comparisons (D67587), and the remaining type references (also D67587). Will update this patch to cover the remaining <> cases.

lnt/external/stats/pstat.py
133 ↗(On Diff #220041)

Changed in D67587.

222 ↗(On Diff #220041)

Changed in D67587.

293 ↗(On Diff #220041)

Changed in D67587.

lnt/server/reporting/runs.py
126 ↗(On Diff #220041)

Fixed in D67582.

lnt/server/ui/views.py
833 ↗(On Diff #220041)

Fixed in D67582.

hubert.reinterpretcast marked 2 inline comments as done.

Reduce patch to updating <> and None comparisons

  • Replaces comparisons using !=, <>, and == against None with the corresponding version of is not None and is None.
  • Replaces <> with !=.
  • As requested by reviewers, add spaces around operators on lines touched.
hubert.reinterpretcast marked 3 inline comments as done.Sep 14 2019, 11:51 AM
hubert.reinterpretcast retitled this revision from [LNT] Python 3 support: Minor mechanical changes to [LNT] Python 3 support: Update `<>` and `None` comparisons.Sep 14 2019, 12:04 PM
hubert.reinterpretcast edited the summary of this revision. (Show Details)
  • Rebase on updated version of D67587

LGTM. Thanks for doing this.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 15 2019, 4:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2019, 4:20 PM