Bug 38957 – scm.py should use self.run instead of run_command

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

scm.py should use self.run instead of run_command

[Adam Barth]

Created attachment 55784 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55784 [details]
Patch

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:103
 +      def run(self, args, cwd=None, input=None, error_handler=None, return_exit_code=False, return_stderr=True, decode_output=True):
Do you just want **kwargs?  Or do you want to manage the default values explicitly?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:105
 +          # FIXME: We should use Executive.
Why not just use Executive() now anyway?  No need to add more clients to the raw version, then we have less to import to this file anyway. :)

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:105
 +          # FIXME: We should use Executive.
By use Executive() i mean just create one here locally, even if it's not stored on self. yet.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:130
 +              print self.run(self.status_command(), error_handler=Executive.ignore_error)
Since you're not using **kwargs, and instead specifying all the defaults, it's more difficult for me to tell if you've changed behavior here.

r=me assuming your intent was not to change behavior.  Please consider the above comments.

[Adam Barth]

> Do you just want **kwargs?  Or do you want to manage the default values explicitly?

Future patches will make run smarter to handle the args.  It's possible we'll want **kwargs at some point though.

> Why not just use Executive() now anyway?  No need to add more clients to the raw version, then we have less to import to this file anyway. :)

I'll do that in a future patch.  I just want to take it slow.

> By use Executive() i mean just create one here locally, even if it's not stored on self. yet.

I understand.

> r=me assuming your intent was not to change behavior.  Please consider the above comments.

Thanks for the comments.  Sorry I'm ignoring all of them.  :P

[Adam Barth]

Comment on attachment 55784 [details]
Patch

Clearing flags on attachment: 55784

Committed r59522: <http://trac.webkit.org/changeset/59522>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.