Closed Bug 822875 Opened 7 years ago Closed 7 years ago

Use 4-space indent in TPS and fix pyflakes warnings

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: jgriffin, Assigned: vignesh.sarma)

Details

Attachments

(2 files, 2 obsolete files)

TPS's Python files should use 4-space indentation instead of 2, and we should fix all the many pyflakes warnings that exist in them.
I would want to know what is needed to be done exactly in this bug.
Can you point me to some code. :-)
(In reply to Jonathan Griffin (:jgriffin) from comment #0)
> TPS's Python files should use 4-space indentation instead of 2, and we
> should fix all the many pyflakes warnings that exist in them.

I would like to work on this bug. Where do i start?
You can download pyflakes (http://pypi.python.org/pypi/pyflakes) and run it on TPS's Python code.  That will flag a bunch of common errors, which can then be fixed.

Additionally, most of the code uses old-style 2-space indents; we should update this to modern 4-space indents while we're at it.

TPS lives at http://mxr.mozilla.org/mozilla-central/source/testing/tps/tps/
(Forgot to hit "Save Changes".)

You'll want to get a checkout of services-central.

https://developer.mozilla.org/en-US/docs/Developer_Guide
https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial?redirectlocale=en-US&redirectslug=Mozilla_Source_Code_%28Mercurial%29#Bundles

(a bundle is at http://ftp.mozilla.org/pub/mozilla.org/firefox/bundles/services-central.hg )


Get it building, and to the point where you can run TPS:

http://hg.mozilla.org/services/services-central/file/default/testing/tps/README


Once you have TPS running, and all tests passing, you can run pyflakes on the files in:

http://hg.mozilla.org/services/services-central/file/default/testing/tps/tps

and fix the reported problems. Make sure TPS still runs and passes afterwards.
This is still assigned to nobody, so can I do it? 
I am familiar with python and have been looking to begin somewhere.

The instruction from :rnewman seem fairly clear too...
there are still some pyflakes error like:

tps/testrunner.py:115: redefinition of function 'mobile' from line 111         
tps/__init__.py:5: 'TPSFirefoxRunner' imported but unused
tps/__init__.py:6: 'TPSTestRunner' imported but unused
tps/__init__.py:7: 'MozHttpd' imported but unused    
tps/phase.py:47: local variable 'returncode' is assigned to but never used

I would like to get some idea about how best to solve them,
Attachment #699114 - Flags: review?(jgriffin)
(In reply to vignesh.sarma from comment #6)
> Created attachment 699114 [details] [diff] [review]
> Diff of updated files for the simple pyflake errors I encounterd.
> 
> there are still some pyflakes error like:
> 
> tps/testrunner.py:115: redefinition of function 'mobile' from line 111      
> 
> tps/__init__.py:5: 'TPSFirefoxRunner' imported but unused
> tps/__init__.py:6: 'TPSTestRunner' imported but unused
> tps/__init__.py:7: 'MozHttpd' imported but unused    
> tps/phase.py:47: local variable 'returncode' is assigned to but never used
> 
> I would like to get some idea about how best to solve them,

Hi Vignesh,

Thanks for the patch!  For the unused imports, you can just remove them.  You can also remove 'returncode' variable that's never used.

The "redefinition of function 'mobile'" error isn't a real problem; it's one way that Python properties can be defined.  It seems to be a pyflakes bug, see http://stackoverflow.com/questions/12563469/fix-pyflakes-dealing-with-property-setter-decorator.  So no action is needed for that error.
Assignee: nobody → vignesh.sarma
Status: NEW → ASSIGNED
Hi Jonathan,

The problem with 
"imported but unused" is that they are in __init__ file, so they might be use in some other place with out specifying the actual file.

One place I found was this,

testing/tps/tps/cli.py
12:from tps import TPSTestRunner

I think this is the only one, so I just update that and removed those imports.

If this is ok i can re-indent the whole module in 4-space and submit the final version.
Attachment #699114 - Attachment is obsolete: true
Attachment #699114 - Flags: review?(jgriffin)
Attachment #699540 - Flags: review?(jgriffin)
Comment on attachment 699540 [details] [diff] [review]
solved all pyflakes errors, expect the one with the bug in pyflakes.

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

Thanks for the patch!  Unfortunately, I gave you some bad advice; I didn't realize that some of the 'unused import' errors were in __init__.py.  Those need to be restored.  Pyflakes does some rather basic code analysis; one of the things it does is check if any imports are unused in a given file.  Imports in __init__.py are not there for use in that file, but for use by other scripts that use that module.

Please restore __init__.py and then we can land this!
Attachment #699540 - Flags: review?(jgriffin) → review-
updated the patch,
if this is ok, I would like to run the module through reindent before submitting the final patch.
Thanks.
Attachment #699540 - Attachment is obsolete: true
Attachment #700178 - Flags: review?(jgriffin)
Comment on attachment 700178 [details] [diff] [review]
updated all valid pyflakes errors

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

Thank you!  In a separate patch, if you feel so inclined, you could reformat all these files to use 4-space indents instead of the current 2-space.
Attachment #700178 - Flags: review?(jgriffin) → review+
Ah, I just saw your comment #10, yes, re-indenting in the same patch is fine too.
This doesn't contain the previous updates, this would have to be applied on the previous patch.

It doesn't change any thing other than re-indent the code.
Thanks.
Attachment #700813 - Flags: review?(jgriffin)
Comment on attachment 700813 [details] [diff] [review]
reindent of the code to 4 space,

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

Thanks for cleaning up the extra whitespace as well.
Attachment #700813 - Flags: review?(jgriffin) → review+
Tests pass with the changes.

http://hg.mozilla.org/services/services-central/rev/4ffaf917d5c0
http://hg.mozilla.org/services/services-central/rev/1e80bc5cbeba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=py][mentor=jgriffin]
https://hg.mozilla.org/mozilla-central/rev/1e80bc5cbeba

FYI, next time you land in services-central, please add "[fixed in services]" to the whiteboard and don't resolve the bug until it is merged into m-c.
Target Milestone: --- → mozilla21
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.