WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38957
scm.py should use self.run instead of run_command
https://bugs.webkit.org/show_bug.cgi?id=38957
Summary
scm.py should use self.run instead of run_command
Adam Barth
Reported
2010-05-11 17:24:11 PDT
scm.py should use self.run instead of run_command
Attachments
Patch
(19.41 KB, patch)
2010-05-11 17:25 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2010-05-11 17:25:28 PDT
Created
attachment 55784
[details]
Patch
Eric Seidel (no email)
Comment 2
2010-05-11 20:23:26 PDT
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
Comment 3
2010-05-14 23:23:19 PDT
> 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 4
2010-05-14 23:29:02 PDT
Comment on
attachment 55784
[details]
Patch Clearing flags on attachment: 55784 Committed
r59522
: <
http://trac.webkit.org/changeset/59522
>
Adam Barth
Comment 5
2010-05-14 23:29:09 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug