WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
87528
webkitpy: change scm.add(), scm.delete() to accept multiple paths
https://bugs.webkit.org/show_bug.cgi?id=87528
Summary
webkitpy: change scm.add(), scm.delete() to accept multiple paths
Dirk Pranke
Reported
2012-05-25 12:34:50 PDT
webkitpy: change scm.add(), scm.delete() to accept multiple paths
Attachments
Patch
(6.83 KB, patch)
2012-05-25 12:37 PDT
,
Dirk Pranke
no flags
Details
Formatted Diff
Diff
add tests
(9.21 KB, patch)
2012-05-25 12:59 PDT
,
Dirk Pranke
no flags
Details
Formatted Diff
Diff
ignore - wrong bug
(11.23 KB, patch)
2012-05-25 14:48 PDT
,
Dirk Pranke
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Dirk Pranke
Comment 1
2012-05-25 12:37:09 PDT
Created
attachment 144124
[details]
Patch
Dirk Pranke
Comment 2
2012-05-25 12:59:16 PDT
Created
attachment 144128
[details]
add tests
Dirk Pranke
Comment 3
2012-05-25 13:04:08 PDT
I'm not actually wild about adding add_list() and delete_list() ... it's kind of crufty. The alternatives would be to change add() and delete() to require lists and update all the calling code, or to change add() and delete() to detect whether the parameter is a single string or a list of strings and do the right thing. I haven't convinced myself that either of those solutions is better, either
Ojan Vafai
Comment 4
2012-05-25 13:06:54 PDT
Comment on
attachment 144128
[details]
add tests View in context:
https://bugs.webkit.org/attachment.cgi?id=144128&action=review
> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1187 > - self._second_local_commt() > + self._second_local_commit()
lol whoops. :)
Dirk Pranke
Comment 5
2012-05-25 13:48:11 PDT
Committed
r118557
: <
http://trac.webkit.org/changeset/118557
>
Dirk Pranke
Comment 6
2012-05-25 14:47:58 PDT
Reopening to attach new patch.
Dirk Pranke
Comment 7
2012-05-25 14:48:01 PDT
Created
attachment 144150
[details]
ignore - wrong bug
Eric Seidel (no email)
Comment 8
2012-05-25 15:01:03 PDT
Comment on
attachment 144150
[details]
ignore - wrong bug View in context:
https://bugs.webkit.org/attachment.cgi?id=144150&action=review
> Tools/Scripts/webkitpy/common/system/executive.py:464 > +def _run_command_thunk(cmd_line_and_cwd): > + (cmd_line, cwd) = cmd_line_and_cwd > + proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) > + stdout, stderr = proc.communicate() > + return (proc.returncode, stdout, stderr)
This could easily have been a class method instead of a module method.
> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:67 > + self._defer_scm_adds = False
It doesn't feel like it's really "defer"ing anything. It's just printing instead of doing.
> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:126 > + print "scm-add:%s" % path
Do you have any testing of this printing and parsing of the resulting values? Spaces in file names? unicode?
> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:194 > + files_to_add = set() > + for output in [result[1] for result in command_results]: > + files_to_add.update(line.replace('scm-add:', '') for line in output.splitlines() if line.startswith('scm-add:')) > + scm.add_list(list(files_to_add))
Yeah, this should be tested. You'll likely have to make it a separate function. The whole idea of serliazing/deserializing the add list is split across two classes currently.
> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:209 > +def run_command(cmd_line_and_cwd): > + (cmd_line, cwd) = cmd_line_and_cwd > + host = Host() > + output = host.executive.run_command(cmd_line, cwd=cwd)
:( You really don't want to instantiate a new host per command.
Eric Seidel (no email)
Comment 9
2012-05-25 15:01:54 PDT
Ok, so where is the right bug so I can add my review comemnts there? :)
Eric Seidel (no email)
Comment 10
2012-05-25 15:20:16 PDT
(In reply to
comment #8
)
> (From update of
attachment 144150
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=144150&action=review
> > > Tools/Scripts/webkitpy/common/system/executive.py:464 > > +def _run_command_thunk(cmd_line_and_cwd): > > + (cmd_line, cwd) = cmd_line_and_cwd > > + proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) > > + stdout, stderr = proc.communicate() > > + return (proc.returncode, stdout, stderr) > > This could easily have been a class method instead of a module method.
Sorry, I should have said "static". @staticmethod def _run_command_thunk(command_line_and_cwd): pass MyClass._run_command_thunk should then do exactly what you want, no?
Eric Seidel (no email)
Comment 11
2012-05-25 15:20:59 PDT
Sorry, I keep getting confused as to which bug when clicking on links in my bugmail.
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