[Webkit-unassigned] [Bug 34439] webkit-patch does not prompt username if the default one is incorrect

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34439


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49015|review?                     |review-
               Flag|                            |




--- Comment #8 from Eric Seidel <eric at webkit.org>  2010-02-19 16:18:58 PST ---
(From update of attachment 49015)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.