[webkit-reviews] review denied: [Bug 34439] webkit-patch does not prompt username if the default one is incorrect : [Attachment 49015] Patch with unit tests

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 34439: webkit-patch does not prompt username if the default one is
incorrect
https://bugs.webkit.org/show_bug.cgi?id=34439

Attachment 49015: Patch with unit tests
https://bugs.webkit.org/attachment.cgi?id=49015&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I would have made this generic:
 307	 def has_webkit_authorization_file(home_directory=os.getenv("HOME")):

not webkit-specific.  Or at least have a generic version.  Since it looks at
the values from SVN itself, it seems no need to have _webkit_ in the name.  It
could use those values as default values to arguments passed to it.

I would have written this using List.extend:
373		svn_commit_args.append('--username')
 374		 svn_commit_args.append(username)
svn_commit_args.extend(["--username", username])

I think we're trying to standardize on " instead of ' anyway.  You can ask Adam
for more infos.

Making this static just makes it harder to mock/unit test:
6     @staticmethod
 307	 def has_webkit_authorization_file(home_directory=os.getenv("HOME")):

I suggest not making it static.

Otherwise I think this looks OK.

Clearly we don't have any unit testing of the actual commit, since the
SVN.has_webkit_authorization_file call would actually be hitting disk for the
real auth file in those cases!

r- for the above nits.