Last Comment Bug 822875 - Use 4-space indent in TPS and fix pyflakes warnings
: Use 4-space indent in TPS and fix pyflakes warnings
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: TPS (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: vignesh.sarma
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-18 15:47 PST by Jonathan Griffin (:jgriffin)
Modified: 2013-01-12 00:27 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Diff of updated files for the simple pyflake errors I encounterd. (1.85 KB, patch)
2013-01-08 04:16 PST, vignesh.sarma
no flags Details | Diff | Review
solved all pyflakes errors, expect the one with the bug in pyflakes. (2.90 KB, patch)
2013-01-08 19:05 PST, vignesh.sarma
jgriffin: review-
Details | Diff | Review
updated all valid pyflakes errors (2.37 KB, patch)
2013-01-09 18:52 PST, vignesh.sarma
jgriffin: review+
Details | Diff | Review
reindent of the code to 4 space, (55.23 KB, patch)
2013-01-10 19:07 PST, vignesh.sarma
jgriffin: review+
Details | Diff | Review

Description Jonathan Griffin (:jgriffin) 2012-12-18 15:47:30 PST
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.
Comment 1 Atul Jangra [:atuljangra] 2012-12-23 18:57:47 PST
I would want to know what is needed to be done exactly in this bug.
Can you point me to some code. :-)
Comment 2 Rohit 2012-12-27 03:34:03 PST
(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?
Comment 3 Jonathan Griffin (:jgriffin) 2012-12-28 09:59:32 PST
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/
Comment 4 Richard Newman [:rnewman] 2012-12-28 12:00:51 PST
(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.
Comment 5 vignesh.sarma 2013-01-08 01:28:37 PST
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...
Comment 6 vignesh.sarma 2013-01-08 04:16:58 PST
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,
Comment 7 Jonathan Griffin (:jgriffin) 2013-01-08 10:44:13 PST
(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.
Comment 8 vignesh.sarma 2013-01-08 19:05:32 PST
Created attachment 699540 [details] [diff] [review]
solved all pyflakes errors, expect the one with the bug in pyflakes.

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.
Comment 9 Jonathan Griffin (:jgriffin) 2013-01-09 12:58:39 PST
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!
Comment 10 vignesh.sarma 2013-01-09 18:52:08 PST
Created attachment 700178 [details] [diff] [review]
updated all valid pyflakes errors

updated the patch,
if this is ok, I would like to run the module through reindent before submitting the final patch.
Thanks.
Comment 11 Jonathan Griffin (:jgriffin) 2013-01-10 12:40:22 PST
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.
Comment 12 Jonathan Griffin (:jgriffin) 2013-01-10 12:41:00 PST
Ah, I just saw your comment #10, yes, re-indenting in the same patch is fine too.
Comment 13 vignesh.sarma 2013-01-10 19:07:14 PST
Created attachment 700813 [details] [diff] [review]
reindent of the code to 4 space,

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.
Comment 14 Jonathan Griffin (:jgriffin) 2013-01-11 11:32:07 PST
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.
Comment 16 Gregory Szorc [:gps] 2013-01-12 00:27:41 PST
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.

Note You need to log in before you can comment on or make changes to this bug.