1155968 - xpcom/idl-parser/ code should follow the flake8 convention

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Run on the Firefox source code, the following line shows a bunch of warnings. Nothing critical but it would be nice to remove them:
$ flake8 --max-line-length=1000 xpcom/idl-parser/

That should be pretty easy to fix for a beginner in Python.
One patch per kind of bug.

Comment 1

[Boris Kudryavtsev [:bkudryavtsev]]

I would like to work on this.

Comment 2

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: bug1155968_removeflake8warnings.diff

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8600669 [details] [diff] [review]
bug1155968_removeflake8warnings.diff

Sorry Boris but as said
"One patch per kind of bug."

Comment 4

[Boris Kudryavtsev [:bkudryavtsev]]

Oh, I see. Will fix. Should I have separate patches for separate files?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

(In reply to Boris Kudryavtsev [:bkudryavtsev] from comment #4)
> Oh, I see. Will fix. Should I have separate patches for separate files?
No, one patch per kind of error.

Comment 6

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_separateimports.diff

Comment 7

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_missingblanklines.diff

Comment 8

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_trailingwhitespace.diff

Comment 9

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_whitespaceinblank.diff

Comment 10

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_multstatementsonline.diff

Comment 11

[Andrew McCreight [:mccr8]]

khuey is probably a better reviewer for XPIDL stuff.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8601290 [details] [diff] [review]
rev1 - bug1155968_missingblanklines.diff

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

FWIW I think the PEP 8 two lines thing here is kind of silly, but ok, consistency wins.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for the patches Boris!

Comment 14

[Boris Kudryavtsev [:bkudryavtsev]]

There are a couple more patches to come. Don't mark this one fixed just yet.

Comment 15

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_semicolons.diff

Comment 16

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_indentations.diff

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8604968 [details] [diff] [review]
rev1 - bug1155968_indentations.diff

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

::: xpcom/idl-parser/header.py
@@ +436,1 @@
>         fd.write('\\')

Er, I think you reindented the wrong thing here.

Comment 18

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev2 - bug1155968_indentations.diff

Is this more like it?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8604991 [details] [diff] [review]
rev2 - bug1155968_indentations.diff

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

Yup.

Comment 20

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_comparisontoNone.diff

Comment 21

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_whitespacefixes.diff

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Boris, you can now request the checkin.

Just add the keyword checkin-needed in the bug.

Comment 23

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: bug1155968_indentfixes.diff

Comment 24

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_initfix.diff

Comment 25

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev1 - bug1155968_emptylines.diff

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8608494 [details] [diff] [review]
rev1 - bug1155968_initfix.diff

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

Why is this ok?  What happens if no iface is found?

Comment 27

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev2 - bug1155968_initfix.diff

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8613086 [details] [diff] [review]
rev2 - bug1155968_initfix.diff

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

This is still going to throw an exception, because None has no location property.

Comment 29

[Boris Kudryavtsev [:bkudryavtsev]]

I am confused on how to fix this. The PEP8 warning I get is: F821 undefined name 'c'.

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Kudryavtsev [:bkudryavtsev] from comment #29)
> I am confused on how to fix this. The PEP8 warning I get is: F821 undefined
> name 'c'.

Yeah, it's complaining that if we take that exception branch we'll try to get c.location before c is defined.  You should just remove the c.location parameter from that IDLError instantiation.

Comment 31

[Boris Kudryavtsev [:bkudryavtsev]]

Attachment: rev3 - bug1155968_initfix.diff

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Boris, I think you can now request the checkin. For this, add checkin-needed in the keyword area.

Comment 33

[Boris Kudryavtsev [:bkudryavtsev]]

Should this be run through try?

Comment 34

[Carsten Book [:Tomcat]]

(In reply to Boris Kudryavtsev [:bkudryavtsev] from comment #33)
> Should this be run through try?

yeah can we get a try run here ? thanks!

Comment 35

[Boris Kudryavtsev [:bkudryavtsev]]

I am currently unable to do a try run. Can someone else do it? Thanks.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This doesn't need to go through try.  Just land it.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaaff73823bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/775890dd46d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e575d9410377
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13499df8cc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3b998c1c6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/48dd997a3d58
https://hg.mozilla.org/integration/mozilla-inbound/rev/441f01e12616
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c9859de2cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b27206136b3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b481796d64b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8277d0a1f7b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/70839bf0a04e

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eaaff73823bf
https://hg.mozilla.org/mozilla-central/rev/775890dd46d3
https://hg.mozilla.org/mozilla-central/rev/e575d9410377
https://hg.mozilla.org/mozilla-central/rev/e13499df8cc9
https://hg.mozilla.org/mozilla-central/rev/fc3b998c1c6f
https://hg.mozilla.org/mozilla-central/rev/48dd997a3d58
https://hg.mozilla.org/mozilla-central/rev/441f01e12616
https://hg.mozilla.org/mozilla-central/rev/72c9859de2cb
https://hg.mozilla.org/mozilla-central/rev/b27206136b3f
https://hg.mozilla.org/mozilla-central/rev/9b481796d64b
https://hg.mozilla.org/mozilla-central/rev/8277d0a1f7b4
https://hg.mozilla.org/mozilla-central/rev/70839bf0a04e