automationutils:parseKeyValue is used in only one file, mochitest's runtests.py: http://dxr.mozilla.org/mozilla-central/search?q=parseKeyValue+ext%3Apy&case=true&redirect=true Let's move it there and delete it from automationutils.py.

I'll like to work on this one.

I made a quick grep search and found that we can also do the same for automationutils:KeyValueParseError , so making the same change here too.

I thought more on this, and realised that automationutils:KeyValueParseError is better placed in utils only. Please let me know what do you think for this ? For the time being I made the rest of the suggested changes. Please Review.

Comment on attachment 8515458 [details] [diff] [review] Bug-1091267.patch Review of attachment 8515458 [details] [diff] [review]: ----------------------------------------------------------------- I think the approach looks good, but there is a bug. Please fix and re-upload. Thanks for the patch! ::: testing/mochitest/runtests.py @@ +1065,5 @@ > self.counttodo = 0 > > self.expectedError = {} > self.result = {} > + nit: extra whitespace here @@ +1066,5 @@ > > self.expectedError = {} > self.result = {} > + > + def parseKeyValue(strings, separator='=', context='key, value: '): This will cause an error, you'll either need to move this function outside of the class definition, or add a 'self' keyword as the first argument (and update callers to use self.parseKeyValue). I'd vote for moving it outside of the class definition and into global scope.

Comment on attachment 8515458 [details] [diff] [review] Bug-1091267.patch Sorry, did that by mistake..

Exactly ! Made the suggested change. Please review.

Comment on attachment 8516066 [details] [diff] [review] Bug-1091267-v2.patch Review of attachment 8516066 [details] [diff] [review]: ----------------------------------------------------------------- Thanks again! :) Please upload a new patch that has the proper author information and a commit message formatted according to [1]. Also if KeyValueParseError is only used in runtests.py, I think it would be good to move it over as well. Feel free to do that in the same patch if you want, I don't mind re-reviewing. Let me know if you have any questions! [1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Moved `KeyValueParseError` also, here is a properly formatted patch. Please review :-)

Comment on attachment 8518094 [details] [diff] [review] Bug-1091267-v3.patch Review of attachment 8518094 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks!

I think this is simple enough that a try run would just be a waste of resources.